Skip to content
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

Fix TestsManifestGeneration tests #46658

Merged
merged 2 commits into from
Jan 7, 2021
Merged

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Jan 6, 2021

Fix #46518

These tests sometimes fail because the call to disable the provider or roll over the file happens almost instantly after TraceEventSession.Flush(), and the Manifest data may not have been fully written to the file before we check for its existence. This adds a sleep between flushing and disabling the provider so that there's plenty of leeway for the payload to be fully written to the trace file.

@ghost
Copy link

ghost commented Jan 6, 2021

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #46518

These tests sometimes fail because the call to disable the provider or roll over the file happens almost instantly after TraceEventSession.Flush(), and the Manifest data may not have been fully written to the file before we check for its existence. This adds a sleep between flushing and disabling the provider so that there's plenty of leeway for the payload to be fully written to the trace file.

Author: sywhang
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

I don't like adding sleeps in tests, but that seems to be the pattern for this test already and as you've pointed out, we need to wait here anyway. Can you decorate the sleep calls you're adding with comments for why they are "necessary"?

@sywhang sywhang merged commit 1d0f67f into dotnet:master Jan 7, 2021
@sywhang sywhang deleted the dev/suwhang/46518 branch January 7, 2021 01:52
@danmoseley
Copy link
Member

danmoseley commented Jan 24, 2021

Coming to this late, but could you not use the pattern of using a nominal (or no) sleep, doing the test, and if it fails, doing the longer sleep and testing again? This is the pattern we use many other places. Per the test history, we know that in the 99% case this sleep is not necessary. With the change as is, the tests all take longer to run, it's not a situation we want to be in.

@sywhang
Copy link
Contributor Author

sywhang commented Jan 24, 2021

@danmosemsft thanks for the suggestion. So these sleeps were removed in a subsequent PR that addressed more issues on the test... But yes, I agree and I think we need to come up with a better synchronization mechanisms for the tracing tests as many of them are timing sensitive.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test_EventSource_EtwManifestGenerationRollover failed in CI
4 participants