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

Use eventFired in buffer-full-add-after-full-event test to avoid flakiness #15849

Closed
hawkinsw opened this issue Mar 14, 2019 · 4 comments · Fixed by #16228
Closed

Use eventFired in buffer-full-add-after-full-event test to avoid flakiness #15849

hawkinsw opened this issue Mar 14, 2019 · 4 comments · Fixed by #16228

Comments

@hawkinsw
Copy link
Contributor

@yoavweiss

I have been looking at buffer-full-add-after-full-event test for a while today and realized that it might be possible to fight the test's flakiness by using the eventFired flag a second time as a more robust way to wait for the random resource to be loaded in clearAndAddAnotherEntryToBuffer. I made the change locally and it seems to work decently. If you would be amenable to fixing the test in such a way, I'd be happy to submit a PR and get your feedback.

Thanks!
Will

@yoavweiss
Copy link
Contributor

The change you have in mind is not 100% clear to me, but a PR of your change might be the best and easiest way to explain it :)

On my end, I haven't seen flakiness since I added an extra task, but admittedly, that's not ideal. Are you still seeing flakiness in the current version? If so, I agree that fixing it is important.

@hawkinsw
Copy link
Contributor Author

Adding @mstange is a person to watch this.

@hawkinsw
Copy link
Contributor Author

The change you have in mind is not 100% clear to me, but a PR of your change might be the best and easiest way to explain it :)

On my end, I haven't seen flakiness since I added an extra task, but admittedly, that's not ideal. Are you still seeing flakiness in the current version? If so, I agree that fixing it is important.

Unfortunately we are still seeing some oddity with the tests. It seems to be timing-related, but honestly I don't know since I'm relatively new to this code.

I am going to send you a quick little PR and see what you think. Like you said, seeing the code is probably the best way to explain :-)

@hawkinsw
Copy link
Contributor Author

hawkinsw commented Apr 2, 2019

@yoavweiss Just FYI: I proposed a change that should resolve the issue. I'd love your thoughts.

yoavweiss pushed a commit that referenced this issue Apr 16, 2019
…#16228)

Resolve #15849 by offering up to five opportunities to
recognize that a new resource has been added to the page
after the buffer full event. This does not change
the semantics of the test but does make it more robust
in the face of timing variability.
marcoscaceres pushed a commit that referenced this issue Jul 23, 2019
…#16228)

Resolve #15849 by offering up to five opportunities to
recognize that a new resource has been added to the page
after the buffer full event. This does not change
the semantics of the test but does make it more robust
in the face of timing variability.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants