Implement e2e tests for export functionality#3506
Implement e2e tests for export functionality#3506DuBento wants to merge 23 commits intothought-machine:masterfrom
Conversation
toastwaffle
left a comment
toastwaffle
left a comment
There was a problem hiding this comment.
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?
| # Golden-Master validation with expected repo. Done before any building to avoid plz-out | ||
| f'diff -r "{exported_repo}" "$DATA_EXPECTED_REPO"', | ||
| # Tests building the exported target which in turn ensures the sources are included | ||
| f'plz {config_override} --repo_root="{exported_repo}" build {targets}', |
There was a problem hiding this comment.
Does it make sense for this to just be plz build //...? Each repo should be tiny, so building everything shouldn't be a problem, and it asserts our requirement that the entire export should be buildable?
|
|
||
| # Export a target generated by a native genrule target and trim unused | ||
| # build statements. | ||
| please_export_e2e_test( |
There was a problem hiding this comment.
Please could you move each of these please_export_e2e_test targets to a BUILD file alongside the test/expected repos?
| @@ -0,0 +1,5 @@ | |||
| filegroup( | |||
There was a problem hiding this comment.
Either this should contain an entry for dummy.build_defs, or dummy.build_defs shouldn't exist
| please_export_e2e_test( | ||
| name = "export_standard_children_custom_target", | ||
| cmd_on_export = [ | ||
| # Child of build def |
There was a problem hiding this comment.
Let's avoid referring to children, as that's not well-defined terminology. adjacent_targets_in_build_def is probably better naming for this case, and the fact that one is a "child" of the other doesn't make a difference.
What you're essentially capturing here is that a target A created in a build def alongside the target B that we're trying to export will result in both A and B being buildable
|
|
||
| # # Test go binary target. | ||
| please_export_e2e_test( | ||
| name = "export_go_binary", |
There was a problem hiding this comment.
Other than the target being exported moving to a subdirectory, I don't see any behavioural difference between this and export_go_target_with_go_dep
| ) | ||
|
|
||
| # Test outputs export. | ||
| please_repo_e2e_test( |
There was a problem hiding this comment.
Why do you not use please_export_e2e_test here? I think both would be clearer as golden-master tests
| ) | ||
|
|
||
| # Export a target from a repo that preloads a build def. | ||
| # This test purposely doesn't use the custom def but checks that the source files are still included. |
There was a problem hiding this comment.
| # This test purposely doesn't use the custom def but checks that the source files are still included. | |
| # The exported target doesn't use the preloaded build def, but the preloaded build def is still present in the export. |
| config_override = "" | ||
| if include_go: | ||
| config_override += "-o plugin.go.gotool:$TOOLS_GO" | ||
| tools["GO"] = [CONFIG.GO.GO_TOOL] |
There was a problem hiding this comment.
is this necessary at all?
|
|
||
| config_override = "" | ||
| if include_go: | ||
| config_override += "-o plugin.go.gotool:$TOOLS_GO" |
There was a problem hiding this comment.
can't we just put this in the .plzconfig in the source repo?
build stmt checker now uses awk, supports more than one stmt associated with with a target and has a strict matching mode
Reverted changes to main test build def
2 participants
This changes introduce small, targeted export test cases. The goal is to use these test for validating upcoming changes to the export logic.
please_export_e2e_testbuild def that is heavily inspired byplease_repo_e2e_testbut branches away to avoid injecting any base configuration files.test_repoand anexpected_repowith the entire files for testing and for validating the export.please_repo_e2e_testwere added to accommodate additional logic.