test: enable marking of failing coverage tests #25671
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,7 +226,8 @@ coverage-test: coverage-build | |
| $(RM) out/$(BUILDTYPE)/obj.target/node_lib/gen/*.gcda | ||
| $(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/*.gcda | ||
| $(RM) out/$(BUILDTYPE)/obj.target/node_lib/src/tracing/*.gcda | ||
| -NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage FLAKY_TESTS="dontcare" \ | ||
| TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) $(COVTESTS) | ||
| $(MAKE) coverage-report-js | ||
| -(cd out && "../gcovr/scripts/gcovr" --gcov-exclude='.*deps' \ | ||
| --gcov-exclude='.*usr' -v -r Release/obj.target \ | ||
|
|
@@ -277,7 +278,7 @@ coverage-run-js: | |
| $(RM) -r out/$(BUILDTYPE)/.coverage | ||
| $(MAKE) coverage-build-js | ||
| -NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage CI_SKIP_TESTS=$(COV_SKIP_TESTS) \ | ||
| TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) jstest | ||
|
There was a problem hiding this comment. I think it would probably make sense to also specify:
There was a problem hiding this comment. based on @refack's comment I'll leave out setting dontcare completely for now. |
||
| $(MAKE) coverage-report-js | ||
|
|
||
| .PHONY: test | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| prefix js-native-api | ||
|
|
||
| # To mark a test as flaky, list the test name in the appropriate section | ||
| # below, without ".js", followed by ": PASS,FLAKY". Example: | ||
| # sample-test : PASS,FLAKY | ||
|
|
||
| [true] # This section applies to all platforms | ||
|
|
||
| [$system==win32] | ||
|
|
||
| [$system==linux] | ||
|
|
||
| [$system==macos] | ||
|
|
||
| [$arch==arm || $arch==arm64] | ||
|
|
||
| [$system==solaris] # Also applies to SmartOS | ||
|
|
||
| [$system==freebsd] | ||
|
|
||
| [$system==aix] | ||
|
|
||
| [$type==coverage] | ||
| test_function/test: PASS,FAIL,CRASH | ||
|
There was a problem hiding this comment. I like this approach to isolating flaky tests a lot 👍 first saw it in the blink codebase. There was a problem hiding this comment. You might consider putting this in https://github.com/nodejs/node/blob/master/test/root.status There was a problem hiding this comment. You learn something new every day. I was not aware of that option. I like having the list in one place so will move there. |
||
| test_general/testFinalizer: PASS,FAIL,CRASH | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
is this just to prevent
FLAKY_TESTSfrom being overridden by an environment variable, so thatjs-native-api.statusis used?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.
by default it is "run" which means it will fail if the tests crashes, this means if the test is marked flaky it will still not be marked as failed.
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.
Can you double check that... Since you are not marking the tests
FLAKY? Maybe we should keep this consistent with other CI target and just usedontcareas an override-able default?Can we now remove the
-prefix?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 was on the fence on overriding the dontcare. I think I'll remove for now.
In terms of removing - I don't want to do that until we have a job running (similar to the linter) in the main regression job. I want to avoid the case were a test goes in that breaks coverage and then somebody else (people keeing coverage going) are expected to fix it. As soon as we get the coverage check as part of the regression PR then we will remove it.
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.
On the dontcare. To clarify I added that when I marked them as FLAKY but that did not actually work because the builds would then always be yellow which I don't think we wanted. Given that we'd probably we probably have to use FAIL or CRASH I removed the setting of dontcare.