Skip to content

Implement e2e tests for export functionality#3506

Open
DuBento wants to merge 33 commits intothought-machine:masterfrom
DuBento:export-tests
Open

Implement e2e tests for export functionality#3506
DuBento wants to merge 33 commits intothought-machine:masterfrom
DuBento:export-tests

Conversation

@DuBento
Copy link
Copy Markdown

@DuBento DuBento commented Mar 24, 2026

This changes introduce small, targeted export test cases. The goal is to use these test for validating upcoming changes to the export logic.

  • Introduced a new please_export_e2e_test build def that is heavily inspired by please_repo_e2e_test but branches away to avoid injecting any base configuration files.
  • Favoured readability by using golden-master tests: for each case we define a test_repo and an expected_repo with the entire files for testing and for validating the export.
  • Non-breaking changes to the generic please_repo_e2e_test were added to accommodate additional logic.

Comment thread test/build_defs/test.build_defs Outdated
Comment thread test/export/repo/test_builtins/BUILD_FILE Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Copy link
Copy Markdown
Contributor

@toastwaffle toastwaffle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ensure that each test repo has at least one target that does not get exported (getting it as close as possible to the edge of being exported). You've got quite a few cases here where the expected repo is identical to the test repo, which doesn't prove anything for the "export as little as possible" goal of exporting. Bonus points for making the e2e enforce that the test repo and expected repo are different.

Also, what's the behaviour around comments in BUILD files - is it worth a test case which exposes that?

Comment thread test/export/please_export_e2e_test.build_defs Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/test_custom_def/expected_repo/build_defs/BUILD_FILE
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/BUILD Outdated
Comment thread test/export/please_export_e2e_test.build_defs Outdated
config_override = ""
if include_go:
config_override += "-o plugin.go.gotool:$TOOLS_GO"
tools["GO"] = [CONFIG.GO.GO_TOOL]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary at all?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not included the tests fail with "go not found in the path" error.
My understanding with this is that we are injecting from the current go config (plz config of the caller) into the test repo, forwarding the go binary from //third_party/go:toolchain.
I based it on this test: https://sourcegraph.com/r/github.com/thought-machine/please/-/blob/test/proto_plugin/BUILD?L10-15

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possible alternative could be to specify a go_toolchain in the source_repo to make it self-contained and closer to a real plz repo. You were probably hinting at that from the beginning. I'll check if this is possible.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.
It works but let me know if this is what you were thinking, I'm slightly concern about tests starting to fail because of changes to go_toolchain (or versions) but I don't see that happening often.

Comment thread test/export/please_export_e2e_test.build_defs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants