-
Notifications
You must be signed in to change notification settings - Fork 386
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
Allow standalone coverlet usage for integration/end-to-end tests using .NET tool driver #991
Allow standalone coverlet usage for integration/end-to-end tests using .NET tool driver #991
Conversation
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.
Looks good!
A thought about that the test subject process must be shut down somehow: could this be tricky to achieve in CI environments without some help from the coverlet test process? I'm thinking about CI scripts like this, in sh
syntax with a hypothetical new flag:
coverlet "/integrationtest" --target "/application/runner.exe" --terminate-test-on-kill &
coverlet_pid=$!
./my_integration_test_suite.sh
kill -TERM $coverlet_pid
wait $coverlet_pid
Based on what I see in e.g. dotnet/coreclr#4309 this would require adding a ProcessExit()
handler in coverlet
that calls process.Kill()
on the test subject. There would be timing issues in this though, since now there's two ProcessExit()
events that must happend quickly, which may make it untenable.
Perhaps the recommendation will have to be that the integration test suite have a way to tell the test subject to shut down.
Added this in guide https://github.com/coverlet-coverage/coverlet/pull/991/files#diff-0ef674889bda556582dddf36bb4f1f71254f7dcfb27a5aeff2f4e5d379582cb1R75 do you think is not enough? Another idea could be provide a code snipped to use to deterministically flush coverage with some reflection like ,
I think that a test suite has got a workflow like start server -> do tests -> asserts -> stop server -> stop test...do you think that's possible keep a process in hang at the end? |
True, it's difficult to say anything more specific than that given how diverse the test subjects can be.
That's a really good idea! If an integration test anyway have to rely on adding a special endpoint or method to shut down, it would help to ensure that all the hit files are written out too given the limitations on
I was thinking it may be a bit fiddly to find the server process to stop, when it is run from coverlet rather than the CI script. This may be very Unix-flavoured of me though, and it would be solved otherwise in a Windows CI environment. |
Understood what you mean...if I should do that I would use a mutex to sync test/server...but it's true...depends on how the tests was thought. We end always at same point, we should use shared memory in a client/server architecture to avoid all problems 😄 |
Indeed :) |
@tonerdo PTAL |
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! Modulo one comment
Co-authored-by: Toni Solarin-Sodara <toni.edward@outlook.com>
The command below on Linux also has the same issue described in #974 still failed after try try with
|
@jeremyLJ can you open a new issue? The reason for this kind of fails are a lot so we need to go deeper on your use case. |
@MarcoRossignoli thanks for the quick feedback. sorry for the randomization... this enhancement is pretty cool~ |
contributes #781
This is an attempt to support end-to-end coverage scenario.
In this case at the end of tests all generated reports should be moved to a folder and merged with report tools like report generator.
@jakubch1 I know that the workflow you described is a bit different(I call it "cold coverage") but at the moment this is the best we can do.
FYI @AbhitejJohn @pavelhorak