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.

Sign up for GitHub

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

Jump to bottom

Make TestPinger more robust. #6844

Merged
merged 4 commits into from Dec 6, 2018
Merged

Make TestPinger more robust. #6844

merged 4 commits into from Dec 6, 2018

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Nov 30, 2018

Instead of trying to prop up servers to ping, just mock out the
requests library.

Fixes #6830

@jsirois
Make TestPinger more robust.
ff2b9f3 
Instead of trying to prop up servers to ping, just mock out the
requests library.

Fixes pantsbuild#6830
Eric-Arellano
Eric-Arellano approved these changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

This is a great improvement! Much better than simply upping the timeout again.

tests/python/pants_test/cache/test_pinger.py Show resolved Hide resolved
cosmicexplorer
cosmicexplorer approved these changes Nov 30, 2018
View reviewed changes
Copy link
Contributor

@cosmicexplorer cosmicexplorer left a comment

Choose a reason for hiding this comment

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

The failure on https://travis-ci.org/pantsbuild/pants/jobs/461886789 is weird, and I'm not sure what system is flaking out there or I would make an issue. This looks fantastic.

tests/python/pants_test/cache/test_pinger.py Show resolved Hide resolved
@jsirois
Copy link
Member Author

jsirois commented Nov 30, 2018

...and I'm not sure what system is flaking out there or I would make an issue

There is no need to understand a flake to file a bug.

Filed #6847

@jsirois
Copy link
Member Author

jsirois commented Nov 30, 2018

Huh - this appears to fail perfectly under CI's python 2.7 and works perfectly under my local python 2.7. Digging...

TansyArron
TansyArron approved these changes Dec 3, 2018
View reviewed changes
Copy link
Contributor

@TansyArron TansyArron left a comment

Choose a reason for hiding this comment

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

I think that any solution relying on time.sleep will suffer the same issues as the original implementation when the CI machines are under heavy load, but its certainly worth a try!

tests/python/pants_test/cache/test_pinger.py Outdated

def callback(_):
if latency < timeout:
time.sleep(latency)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this will result in the same errors as the original. The flake in these tests was always due to time.sleep behaving unpredictably on CI machines under heavy load.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - that makes sense. This is only needed for 1 of the test methods - let me see what I can do to eliminate this outright.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed - PTAL

@jsirois
Copy link
Member Author

jsirois commented Dec 5, 2018

OK - the error is beacuse of an api difference in responses for declarative mocking vs mocking with callbacks: getsentry/responses#235 ... I am not sure how this passes on python3! That said - I can fix by using the current undocumented API.

baroquebobcat
baroquebobcat approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@baroquebobcat baroquebobcat left a comment

Choose a reason for hiding this comment

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

Looks good to me! Added suggestions for a typo

tests/python/pants_test/cache/test_pinger.py Outdated Show resolved Hide resolved
tests/python/pants_test/cache/test_pinger.py Outdated Show resolved Hide resolved
tests/python/pants_test/cache/test_pinger.py Outdated Show resolved Hide resolved
Eric-Arellano
Eric-Arellano approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

These changes look great! Thanks Tansy for catching the underlying issue and John for reworking this.

tests/python/pants_test/cache/test_pinger.py Outdated Show resolved Hide resolved
@jsirois jsirois mentioned this pull request Dec 6, 2018
Closed
@jsirois jsirois merged commit 16d1c7e into pantsbuild:master Dec 6, 2018
@jsirois jsirois deleted the rust/ui/propagate_flush_errors branch December 6, 2018 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baroquebobcat baroquebobcat baroquebobcat approved these changes

@stuhood stuhood stuhood approved these changes

@cosmicexplorer cosmicexplorer cosmicexplorer approved these changes

@TansyArron TansyArron TansyArron approved these changes

@Eric-Arellano Eric-Arellano Eric-Arellano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jsirois @baroquebobcat @stuhood @cosmicexplorer @TansyArron @Eric-Arellano