-
Notifications
You must be signed in to change notification settings - Fork 721
Added PISTACHE_ENABLE_FLAKY_TESTS option, defaulting to True, to toggle unit tests that are known to be flaky... #1051
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
Conversation
…le unit tests that are known to be flaky...
|
@Tachi107 it looks like all tests either explicitly failed or were false negatives. The explicitly failed tests, like on
I don't know why because that's been added to As for those that ought to have failed, as an example on Thoughts? |
That's probably because you opened a PR from a branch under the pistacheio namespace and the checkout CI action is cloning the master branch instead of yours. I've never tested this, as I usually branch in my personal repo instead, but the issue should go away when merging this into master.
Unfortunately that's an issue I've never been able to solve. Something's wrong in either GitHub's Ubuntu image or autopkgtest, and virtualized runs often fail for no apparent reason. |
|
Got it. Ok, let's wait and see what the Launchpad builders do now. |
|
All builds ok! |
|
|
||
| if get_option('PISTACHE_ENABLE_FLAKY_TESTS') | ||
| pistache_test_files += 'streaming_test' | ||
| pistache_test_files += 'rest_swagger_server_test' | ||
| endif | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have instead suggested defining these as test(..., suite: ['flaky']), so that people can select the tests at will with meson test --no-suite flaky. It is also possible to make meson test without arguments default to not running a suite:
add_test_setup(
'noflaky',
exclude_suites: ['flaky'],
is_default: true, # run this one without `meson test --setup noflaky`
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's cool, and certainly better than having to reconfigure your build to skip flaky tests.
@kiplingw for this to work in the Ubuntu PPA we would need to bump debhelper-compat to version 13, as version 12 and below run ninja test instead of meson test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add_test_setup() works well with ninja test too. :)
(No idea how debian packaging works, so no clue whether bumping debhelper can cause compat issues.)
EDIT: though unfortunately it has the opposite problem -- it raises the minimum meson to 0.57 for exclude_suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it would force us to skip flaky tests by default, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eli-schwartz, I think your solution is much more elegant. If you'd like to submit a PR, I'm totally fine with that.
@Tachi107, yes, I recall we'd have to bump the debhelper-compat version. I'm fine with that, but will that break all unit tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Tachi107, yes, I recall we'd have to bump the debhelper-compat version. I'm fine with that, but will that break all unit tests?
No, but it might break compatibility with old Ubuntu releases. I don't know if the build tools of older Ubuntu versions get upgraded to support newer debhelper-compat versions. But we could at least try
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to give it a try?
I also think the approach of flagging flaky tests as flaky in the build environment is much better than what I proposed. That way they still run, but the entire build environment doesn't break in the interim while we wait for fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the approach of flagging flaky tests as flaky in the build environment is much better than what I proposed. That way they still run, but the entire build environment doesn't break in the interim while we wait for fixes.
No, they wouldn't run, but you would be able to easily skip them with --no-suite flaky. An alternative that would run still run them but wouldn't mark their failure as such would be to mark them with should_fail: true.
I've submitted a PR that implements what Eli suggested as #1052
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok perfect. If you could add to your PR to override override_dh_auto_test in d/rules so that --no-suite flaky is added, then it should work.
Added PISTACHE_ENABLE_FLAKY_TESTS option, defaulting to True, to toggle unit tests that are known to be flaky. This is a temporary workaround for failing CI created by #1023 and #1033.