Skip to content

qa: Ignore --failfast option of test_runner.py on Windows #22980

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
Closed
hebasto wants to merge 1 commit into from
Closed

qa: Ignore --failfast option of test_runner.py on Windows #22980

hebasto wants to merge 1 commit into from

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 15, 2021

The current --failfast implementation does not work on Windows.

This PR is an alternative to #22936, and it is based on the #22936 (review):

An alternative would be to just not fix it. I mean who uses Windows anyway (to run the functional tests with --failfast).

At least with the error message, it well be clear that something went wrong. With this patch it will leave them dangling.

@hebasto
qa: Ignore --failfast option of test_runner.py on Windows
4da15f9 
The current `--failfast` implementation does not work on Windows.
@DrahtBot DrahtBot added the Tests label Sep 15, 2021
@maflcko
Copy link
Member

maflcko commented Sep 15, 2021

Again, it would be preferable to not fixing anything here. Am I missing something?

With this patch we will waste CI cycles after hitting a failure.

To quote myself:

An alternative would be to just not fix it. I mean who uses Windows anyway (to run the functional tests with --failfast).

@hebasto hebasto closed this Sep 15, 2021
@hebasto hebasto mentioned this pull request Sep 24, 2021
Closed
@hebasto
Copy link
Member Author

hebasto commented Nov 9, 2021 •
edited
Loading

@MarcoFalke

With this patch we will waste CI cycles after hitting a failure.

A failure in functional tests is rare.

OTOH, seeing all of the failed tests can give us a useful hint for debugging (was thinking about that while reviewing and testing #23300).

For reasons mentioned above, I'd disable --fail-fast in all CI tasks.

@maflcko
Copy link
Member

maflcko commented Nov 9, 2021

Yeah, it is a trade-off. If there is a CI failure, it will be good to know the result as early as possible after opening the pull request. If there is more time, it will be good to know all tests that failed.

Ideally the ci infrastructure could be marked red during a run. Then the run would continue with the remaining tests.

@hebasto
Copy link
Member Author

hebasto commented Nov 9, 2021

it will be good to know the result as early as possible after opening the pull request.

With high probability a first failed test won't be the first in the run queue. Also it could be one of EXTENDED_SCRIPTS.

Therefore, the average amount of saved time for such an approach is less significant than it could appear.

@maflcko
Copy link
Member

maflcko commented Nov 9, 2021

Well that would be a reason to simply remove the option, thus revert #13105.

@hebasto hebasto mentioned this pull request Nov 15, 2021
Merged
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Nov 15, 2021
Merge bitcoin/bitcoin#23513: ci: Drop --failfast in functional tests …
15d1098 
…on native Windows CI task

23c3dcb ci: Drop --failfast in functional tests on native Windows CI task (Hennadii Stepanov)

Pull request description:

  As it was [discussed](bitcoin/bitcoin#22980 (comment)) in bitcoin/bitcoin#22980:
  > seeing _all_ of the failed tests can give us a useful hint for debugging (was thinking about that while reviewing and testing bitcoin/bitcoin#23300).

  There is a [concern](bitcoin/bitcoin#22980 (comment)) about such approach:
  > If there is a CI failure, it will be good to know the result as early as possible after opening the pull request.

  But, [OTOH](bitcoin/bitcoin#22980 (comment)):
  > the average amount of saved time for such an approach [using `--failfast`] is less significant than it could appear.

ACKs for top commit:
  MarcoFalke:
    cr ACK 23c3dcb seems fine to give this a try

Tree-SHA512: d28f5712b4edfdbcef48b0633017da9172cef1835bcea51eaeeabf15c133f6bb6888227afc130279c3899365a4fd0f200fb9b0c4cb1ff80f21cbc766b8907764
@bitcoin bitcoin locked and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hebasto @maflcko @DrahtBot