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

Remove commented out pytest-rerunfailures test dependency #3263

Merged
merged 1 commit into from
Feb 26, 2022

Conversation

pabs3
Copy link
Contributor

@pabs3 pabs3 commented Nov 7, 2021

pytest-rerunfailures is now compatible with pytest 6.1.0 and later.

Reverts: commit 8687e7f.
Reported-in: pytest-dev/pytest-rerunfailures#128
Fixed-by: pytest-dev/pytest-rerunfailures#129

@codecov
Copy link

codecov bot commented Nov 7, 2021

Codecov Report

Merging #3263 (c43b365) into develop (6fc9e38) will not change coverage.
The diff coverage is n/a.

❗ Current head c43b365 differs from pull request most recent head 6f7e9f5. Consider uploading reports for the commit 6f7e9f5 to get more accurate results

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3263   +/-   ##
========================================
  Coverage    79.53%   79.53%           
========================================
  Files           68       68           
  Lines        11781    11781           
========================================
  Hits          9370     9370           
  Misses        2411     2411           
Impacted Files Coverage Δ
gensim/models/doc2vec.py 81.61% <0.00%> (ø)

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 d6620df...6f7e9f5. Read the comment docs.

@mpenkov
Copy link
Collaborator

mpenkov commented Dec 4, 2021

What's the benefit of re-running failures?

In my opinion, if a test keeps blinking, it's probably a bad test. We should improve the test so that it does not blink instead of re-running it and hoping that we get lucky on one of the attempts.

@pabs3 What are your thoughts?

@pabs3
Copy link
Contributor Author

pabs3 commented Dec 4, 2021 via email

@mpenkov
Copy link
Collaborator

mpenkov commented Dec 4, 2021

@menshikh-iv @piskvorky What do you guys think? Can we get rid of this and run tests once only?

@piskvorky
Copy link
Owner

Do we still experience flaky tests? How bad is it – if not bad, then no reason to automate "re-runs" IMO.

@mpenkov
Copy link
Collaborator

mpenkov commented Dec 5, 2021

I don't think it's bad. One of the wheel builds fails occasionally, but there are ways around that other than automatic re-runs (plus the re-runs don't always help).

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Dec 6, 2021

If I remember correctly, rerun plugin help us to avoid "blinking" test failures, if we didn't see these tests now - we can just don't use this plugin.

there are ways around that other than automatic re-runs

@mpenkov just curious, would you mind to add details?

@mpenkov
Copy link
Collaborator

mpenkov commented Dec 6, 2021

In order of decreasing preference, we could:

  • Eliminate the external factors that cause the test to blink (are we forgetting to mock or otherwise nail down something?)
  • Rewrite the test so that it does not blink (why do these test blink, anyway? Where are we dealing with non-deterministic code?)
  • Mark the test as an expected failure under certain conditions
  • Disable the test
  • Remove the test

@gojomo
Copy link
Collaborator

gojomo commented Dec 14, 2021

Personally, I'd rather we default to no automatic rerunning of tests. It slows down detection of reliable failures, & might hide other stability issues better solved another way.

@pabs3 pabs3 force-pushed the re-enable-pytest-rerunfailures branch from 5bca10c to c43b365 Compare February 6, 2022 06:44
@pabs3 pabs3 changed the title Enable pytest-rerunfailures test dependency again Remove commented out pytest-rerunfailures test dependency Feb 6, 2022
@pabs3
Copy link
Contributor Author

pabs3 commented Feb 6, 2022

The consensus seems to be to not re-add pytest-rerunfailures, so I have added a commit removing the commented out dependency.

@piskvorky piskvorky added this to the 4.1.0 milestone Feb 6, 2022
@pabs3
Copy link
Contributor Author

pabs3 commented Feb 20, 2022

Can someone re-run the checks? Seems like one of them timed out and another I cannot tell why the failure happened.

@piskvorky
Copy link
Owner

The wheel-build failures are likely unrelated. @mpenkov is looking into it – thanks for your PR!
The new release is coming soon now :)

@pabs3 pabs3 force-pushed the re-enable-pytest-rerunfailures branch from c43b365 to 6f7e9f5 Compare February 26, 2022 06:18
@mpenkov mpenkov merged commit 86b1832 into piskvorky:develop Feb 26, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Feb 26, 2022

Merged. Thank you @pabs3 !

@pabs3 pabs3 deleted the re-enable-pytest-rerunfailures branch February 26, 2022 07:30
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

5 participants