-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
testscript: merge coverage from all test binary executions #119
Conversation
Please do not review yet. I'm still iterating on this. |
d091f99
to
f385bc7
Compare
Alright, this is ready for a first round of review. Note that this change lacks tests and docs, because I want to know if the overall design is sane before I polish it. The main use case is testing tools which are executed indirectly, for example via 'go build -toolexec=mytool'. It is possible to set up It's fine if you think the above is a hack, or that testscript isn't designed for this. However, it nearly works with minimal effort, except for coverage profiles. The basic idea with this feature is that, if the test package's Another way to design this API would be to pass some sort of parameter to I did not try to design this API on a per-script basis since it makes little sense; I also exposed a way for |
We discussed this offline, and came up with a new design that I'll implement over the next week or so. |
f385bc7
to
fdb2a52
Compare
eebd5f2
to
9c7658d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great - it's actually simpler than I expected and should "just work" for people.
LGTM modulo a few comments and suggestions.
6e3409e
to
23ec8ea
Compare
Thanks for your review! I've applied nearly all your suggestions, minus two - I've left those threads unresolved. I've also added a long commit message that summarizes the whole refactor. |
55b027e
to
f4b83c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks! (with a couple of trivial final suggestions)
85c8689
to
21f94f0
Compare
First, consolidate how top-level commands registered via RunMain are executed. Before, they could only be run directly, such as "foo <args>". This worked because the command would run the test binary with a special TESTSCRIPT_COMMAND env variable, telling the sub-process what command to run. The unfortunate side effect was that the commands only worked when called directly. They wouldn't work when called indirectly, such as "exec foo" or "go build -toolexec=foo". To fix that inconsistency, we now set up a directory in $PATH with all the commands as copies of the test binary. The test binary sub-process knows what command it should execute thanks to os.Args[0]. This also lets us get rid of the TESTSCRIPT_COMMAND dance. Second, make all top-level command executions write coverage profiles if the -coverprofile test flag was used. Similar to the case before, this only used to work for direct command executions, not indirect ones. Now they all get merged into the final coverage profile. This is accomplished by having them all write coverage profiles under a shared directory. Once all scripts have run, the parent process walks that directory and merges all profiles found in it. Third, add a test that ensures both of the refactors above work well. It lives under gotooltest, since it runs the real "go test -coverprofile". It checks all three ways mentioned above to run a top-level command, as well as checking that all three count towards the total coverage. Note that some tests needed to be amended to keep working after the refactor. For example, some tests used a custom "echo" command as well as the system's "exec echo". Since now both of those would execute the testscript command, rename that command to fprintargs, which is also clearer and less ambiguous. Similarly, dropgofrompath had to be used in one more test, and had to be adapted to actually do the intended thing on Windows rather than just emptying the entire PATH variable. Also swap Go's 1.16beta1 for 1.16rc1 in CI, since the former is now failing due to a bug in setup-go.
21f94f0
to
854958b
Compare
(see commit message)