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

same health-check ignoring for all profiles #361

Merged
merged 2 commits into from May 25, 2021

Conversation

meejah
Copy link
Collaborator

@meejah meejah commented May 25, 2021

Moved from discussion on an unrelated PR. @tomprince notes, "My thinking is that we want magic-folder-fast to be fast, and if something is triggering this health check, it isn't fast."

@meejah
Copy link
Collaborator Author

meejah commented May 25, 2021

There's at least one test that triggers that error locally consistently (maybe not 100%?) for me so it kind of makes magic-folder-fast not as usable..(the intent of the -fast one was to run locally as quickly as possible, yes, but also good to be closer to CI)...

@meejah meejah mentioned this pull request May 25, 2021
@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #361 (4e55bbb) into main (bb2e5c8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #361   +/-   ##
=======================================
  Coverage   88.69%   88.69%           
=======================================
  Files          31       31           
  Lines        2326     2326           
  Branches      276      276           
=======================================
  Hits         2063     2063           
  Misses        181      181           
  Partials       82       82           
Flag Coverage Δ
integration 49.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb2e5c8...4e55bbb. Read the comment docs.

@tomprince
Copy link
Contributor

There's at least one test that triggers that error locally consistently (maybe not 100%?) for me so it kind of makes magic-folder-fast not as usable..(the intent of the -fast one was to run locally as quickly as possible, yes, but also good to be closer to CI)...

We could fix that error, instead of turning off the health check (and add a job that runs with that flag in CI to ensure it keeps working). Not sure how much work fixing that test is. Alternatively, we could even suppress the health check for that one test (and still add a CI job), if it is too much work to fix that one test right now.

@meejah
Copy link
Collaborator Author

meejah commented May 25, 2021

I don't remember why we decided that extending the deadline was the right thing to do (@exarkun do you recall?) .. looks like it was here 6ae5b65

No matter that, though, I just want a profile that runs fewer examples.

Fixing whatever "deadline" problems we have is separate to that. Of course, it might be good to fix those too .. but also why is Hypothesis' arbitrary deadline something we have to worry about?

@exarkun
Copy link
Member

exarkun commented May 25, 2021

I think that the Hypothesis deadline / too-slow health checks are marginal features at best. My laptop is not a realtime platform and I doubt anyone else's is either. Sometimes Chrome decides to VACUUM 50 SQLite3 databases. Sometimes I check the weather and accidentally run 20 MB of arbitrary ad-placement JavaScript. Those things can cause Hypothesis to think the test suite is unhealthy. It is true that a poorly written test (or maybe even a well-written test for a poorly written implementation) can also cause Hypothesis to think the test suite is healthy. But I have no visibility into what has actually happened in any particular case so I will (approximately) always assume it is a noisy neighbor and not the SUT. Maybe sometime we can improve the situation but it seems unlikely we can do much about it now other than turning off this Hypothesis feature.

Maybe some of that explanation belongs in the code?

@tomprince
Copy link
Contributor

always assume it is a noisy neighbor and not the SUT.

@meejah was suggesting that it was one test which is consistently triggering the error, which suggests that it more likely to be the test than might otherwise be the case.

@exarkun
Copy link
Member

exarkun commented May 25, 2021

always assume it is a noisy neighbor and not the SUT.

@meejah was suggesting that it was one test which is consistently triggering the error, which suggests that it more likely to be the test than might otherwise be the case.

Yea, that could be. Or the test could be closer to the hard cutoff point than most other tests and then noisy neighbors push it over the line with high probability.

@meejah
Copy link
Collaborator Author

meejah commented May 25, 2021

So can I merge this, and if we care about hypothesis health-checks etc, that's followup / different Issue or PR?

(If we do care / fix whatever causes arbitrary deadlines to be exceeded, then we should also change both profiles to accept deadlines). @tomprince does the existing comment in the profile code need more of @exarkun 's words from above?

@meejah meejah merged commit 3a926aa into tahoe-lafs:main May 25, 2021
@meejah meejah deleted the fast-health-check branch May 25, 2021 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants