Skip to content

dev: Add coverage variation to deal with flakiness - #335

Merged
mihaic merged 2 commits into
brainiak:masterfrom
mihaic:adjust-codecov-settings
Mar 19, 2018
Merged

mihaic merged 2 commits into
brainiak:masterfrom
mihaic:adjust-codecov-settings

Conversation

@mihaic

@mihaic mihaic commented Feb 14, 2018

Copy link
Copy Markdown
Member

@mihaic
mihaic requested a review from danielsuo February 20, 2018 18:15
@gdoubleyew

Copy link
Copy Markdown
Contributor

Mihai, I'm not familiar with codecov, but looking at the docs it says "threshold: allowed to drop X% and still result in a "success"".
So by adding threshold 1, are you allowing 1% test failures to still be a success?
You mention occasional multiprocess test flakiness do to some randomness, do you use the @flaky decorator?

@mihaic

mihaic commented Mar 19, 2018

Copy link
Copy Markdown
Member Author

Thanks for having a look at this PR, @gdoubleyew.

Codecov is not looking at directly at test results. In fact, it only runs if the tests were successful. So the threshold only allows the test coverage to drop, it does not allow tests to fail.

The flakiness does not cause test failures in BrainIAK, as far as I can tell. Therefore, we do not need the @flaky decorator to pass tests. Instead, the flakiness causes the coverage to vary. Using @flaky with min_passes > 1 will decrease the chance of a drop in coverage. However, even with a high min_passes, there is still a chance that coverage appears to drop, so we would still see spurious failures. Therefore, I think it is best to introduce the threshold.

@danielsuo danielsuo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me; the 1% drop seems to work, based on the test reports.

@mihaic
mihaic merged commit b51f831 into brainiak:master Mar 19, 2018
@mihaic
mihaic deleted the adjust-codecov-settings branch March 19, 2018 19:03
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.

3 participants