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

test: enable marking of failing coverage tests #25671

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@mhdawson
Copy link
Member

mhdawson commented Jan 23, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Enable marking of coverage tests so that we can
allow some tests to fail without blocking the generation
of coverage data. This will later allow us to
fail the coverage job if other kinds of errors occur and
to capture which tests we believe are not running properly
with coverage enabled.

This is step 1 of the following

  • allow us to run coverage and have failing tests not block generation of results
  • add support for a coverage level check (ex 90%) that will cause a failure @bcoe is working on that
  • add job to main PR regression test that will run coverage to make sure new failing tests are
    not introduced
  • Update makefile so that we don't use '-' for the coverage target. At this point we will fail is
    some other problem occurs but not if one of the known tests that affects coverage fails.

Note that this re-uses the '--type' option that we used for fips testing so that we can have entries
in the status files. The downside is that we would not be able to run coverage on FIPs as they
would conflict trying to use different types. I could add another option '--test-type' but I was
not sure if the added code/complexity is what people would want so I started with this approach.

mhdawson added some commits Jan 22, 2019

test: enable marking of failing coverage tests
Enable marking of coverage tests so that we can
allow some tests to fail without blocking the generation
of coverage data. This will later allow us to
fail the coverage job if other kinds of errors occur and
to capture which tests we believe are not running properly
with coverage enabled.
squash: allow FAIL and CRASH to avoid yellow
Allow FAIL and CRASH to avoid yellow, otherwise
if we added to regular CI run we'd have perma-yellow
squash: allow new tests to pass until ci job added
Allow new tests to fail until we get a ci job added
to the main regression job so that comits don't break
coverage that have to be fixed later on.

@mhdawson mhdawson requested review from bcoe and refack and removed request for bcoe Jan 23, 2019

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 23, 2019

FYI , job in the CI that I've been using for testing and that we'd want to add to the main regression PR.

https://ci.nodejs.org/job/node-test-commit-coverage/

One thing is which machines to run it on, currently coverage requires patch which does not seem to be installed on all of our machines. Maybe just run it on the machines we using for linting? @refack what do you think?

@bcoe

bcoe approved these changes Jan 24, 2019

Copy link
Member

bcoe left a comment

mainly just had some questions, thank you for doing this.

[$system==aix]

[$type==coverage]
test_function/test: PASS,FAIL,CRASH

This comment has been minimized.

@bcoe

bcoe Jan 24, 2019

Member

I like this approach to isolating flaky tests a lot 👍 first saw it in the blink codebase.

This comment has been minimized.

@refack

refack Jan 24, 2019

Member

You might consider putting this in https://github.com/nodejs/node/blob/master/test/root.status
My intuition is that file fit better to represent this is a cross-cutting concern.

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

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.

Makefile Outdated
@@ -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 $(MAKE) $(COVTESTS)
-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage FLAKY_TESTS="dontcare" \

This comment has been minimized.

@bcoe

bcoe Jan 24, 2019

Member

is this just to prevent FLAKY_TESTS from being overridden by an environment variable, so that js-native-api.status is used?

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

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.

This comment has been minimized.

@refack

refack Jan 24, 2019

Member
  1. 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 use dontcare as an override-able default?

  2. Can we now remove the - prefix?

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

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.

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

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.

@@ -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) \
$(MAKE) jstest
TEST_CI_ARGS="$(TEST_CI_ARGS) --type=coverage" $(MAKE) jstest

This comment has been minimized.

@bcoe

bcoe Jan 24, 2019

Member

I think it would probably make sense to also specify:

FLAKY_TESTS="dontcare"

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

k will add.

This comment has been minimized.

@mhdawson

mhdawson Jan 24, 2019

Author Member

based on @refack's comment I'll leave out setting dontcare completely for now.

@bcoe bcoe referenced this pull request Jan 24, 2019

Closed

test: allow coverage threshold to be enforced #25675

2 of 2 tasks complete
@refack

refack approved these changes Jan 24, 2019

Copy link
Member

refack left a comment

Left some questions

Makefile Outdated
@@ -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 $(MAKE) $(COVTESTS)
-NODE_V8_COVERAGE=out/$(BUILDTYPE)/.coverage FLAKY_TESTS="dontcare" \

This comment has been minimized.

@refack

refack Jan 24, 2019

Member
  1. 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 use dontcare as an override-able default?

  2. Can we now remove the - prefix?

[$system==aix]

[$type==coverage]
test_function/test: PASS,FAIL,CRASH

This comment has been minimized.

@refack

refack Jan 24, 2019

Member

You might consider putting this in https://github.com/nodejs/node/blob/master/test/root.status
My intuition is that file fit better to represent this is a cross-cutting concern.

@refack refack added the coverage label Jan 24, 2019

mhdawson added some commits Jan 24, 2019

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 24, 2019

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 25, 2019

Opened issue for un-related failure on Windows: #25702

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 25, 2019

Created issue for unrelated issue on ARM - #25704

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 25, 2019

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 25, 2019

Resume again to try and get passed the arm issue: https://ci.nodejs.org/job/node-test-pull-request/20336/

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 28, 2019

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 28, 2019

I don’t think the ARM issue is resolvable through Resume CIs, sadly 😕

Rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/20388/

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Jan 29, 2019

Landed in c06653e.

@danbev danbev closed this Jan 29, 2019

danbev added a commit that referenced this pull request Jan 29, 2019

test: enable marking of failing coverage tests
Enable marking of coverage tests so that we can
allow some tests to fail without blocking the generation
of coverage data. This will later allow us to
fail the coverage job if other kinds of errors occur and
to capture which tests we believe are not running properly
with coverage enabled.

PR-URL: #25671
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>

targos added a commit that referenced this pull request Jan 29, 2019

test: enable marking of failing coverage tests
Enable marking of coverage tests so that we can
allow some tests to fail without blocking the generation
of coverage data. This will later allow us to
fail the coverage job if other kinds of errors occur and
to capture which tests we believe are not running properly
with coverage enabled.

PR-URL: #25671
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>

@targos targos referenced this pull request Jan 29, 2019

Merged

v11.9.0 proposal #25802

@mhdawson

This comment has been minimized.

Copy link
Member Author

mhdawson commented Jan 30, 2019

@addaleax thanks. So that I know for the future is there a way to tell when an error won't work through resume CI or does that apply for ARM in general?

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 30, 2019

@mhdawson I have no idea – I think the reason is that resume builds might not do an additional rebase against master, where the test had been fixed in the meantime…?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment