Skip to content
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: revert fail coverage target if tests fail" #25543

Closed
wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Jan 16, 2019

This reverts commit f216d5b.
Seems like it breaks the nightly job so reverting until
we figure that out.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This reverts commit f216d5b.
Seems like it breaks the nightly job so reverting until
we figure that out.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 16, 2019
@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label Jan 16, 2019
@mhdawson
Copy link
Member Author

This needs to be reverted as we do have failing tests when run with coverage enabled (and have forever I think) so the original change prevents the gathering and printing of coverage information.

@mhdawson mhdawson changed the title DO NOT LAND STILL VALIDATING test: revert fail coverage target if tests fail" test: revert fail coverage target if tests fail" Jan 16, 2019
@mhdawson
Copy link
Member Author

There is something up with the main coverage job as well, but testing locally I confirmed we need the revert

@mhdawson mhdawson removed the wip Issues and PRs that are still a work in progress. label Jan 17, 2019
@mhdawson
Copy link
Member Author

Would also like to fast track this.

@mhdawson mhdawson added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 17, 2019
@mhdawson
Copy link
Member Author

@mhdawson
Copy link
Member Author

Coverage job CI run, shows that this resolves the issue: https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/2/

@mhdawson
Copy link
Member Author

Renamed jobs as something was broken with the CI job for coverage even though coverage build locally. New CI which references the updated job: https://ci.nodejs.org/view/Node.js%20Daily/job/node-test-commit-linux-coverage/3/

@mhdawson
Copy link
Member Author

mhdawson commented Jan 17, 2019

Windows Node.js CI failure was known issue:#25512

Will resume build.

@mhdawson
Copy link
Member Author

@BridgeAR
Copy link
Member

I marked this as do not land as the original commit is marked as such.

@mhdawson
Copy link
Member Author

@BridgeAR thanks.

@mhdawson
Copy link
Member Author

Strangely renaming a working job, with no other changes caused the job to fail so there is some state somewhere. I've cleared out the workspace, deleted the files on disk but that still did not help. I'm just going to leave it as the new name with "-daily" in the job name.

@mhdawson
Copy link
Member Author

mhdawson commented Jan 17, 2019

This link is now valid again https://ci.nodejs.org/job/node-test-commit-linux-coverage-daily/2/

@mhdawson
Copy link
Member Author

Ok resumed CI job is green :).

@mhdawson
Copy link
Member Author

Think I've got the ok's for fast-track, landing now.

@mhdawson
Copy link
Member Author

Landed in 1375af2

mhdawson added a commit that referenced this pull request Jan 17, 2019
This reverts commit f216d5b.
Seems like it breaks the nightly job so reverting until
we figure that out.

PR-URL: #25543
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@mhdawson mhdawson closed this Jan 17, 2019
@refack
Copy link
Contributor

refack commented Jan 17, 2019

This needs to be reverted as we do have failing tests when run with coverage enabled (and have forever I think)

That's sounds like a sub-optimal situation. I'll try to look into that.

Cross-ref: #25432

@refack
Copy link
Contributor

refack commented Jan 17, 2019

Strangely renaming a working job, with no other changes caused the job to fail so there is some state somewhere.

P.S. renaming the job the same name as the broken one caused Jenkins to reuse the broken workspace directory. It might store a broken compilation artifact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants