FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: revert fail `coverage` target if tests fail" by mhdawson · Pull Request #25543 · nodejs/node · GitHub

/ node Public

test: revert fail coverage target if tests fail" - #25543

Closed
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:revertcov
ConversationCommits (1)ChecksFiles changed
Closed

test: revert fail coverage target if tests fail"#25543
mhdawson wants to merge 1 commit into
nodejs:masterfrom
mhdawson:revertcov

Conversation

mhdawson commented Jan 16, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

test: revert fail coverage target if tests fail"
adbad47 
This reverts commit f216d5b.
Seems like it breaks the nightly job so reverting until
we figure that out.

nodejs-github-bot commented Jan 16, 2019

Copy link
Copy Markdown
Collaborator

@mhdawson build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2271/pipeline

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 16, 2019
mscdex added the wip Issues and PRs that are still a work in progress. label Jan 16, 2019

mhdawson commented Jan 16, 2019

Copy link
Copy Markdown
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 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 commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

mhdawson mentioned this pull request Jan 16, 2019
Closed
mhdawson removed the wip Issues and PRs that are still a work in progress. label Jan 17, 2019

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

Would also like to fast track this.

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

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

CI run: https://ci.nodejs.org/job/node-test-pull-request/20181/

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
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 commented Jan 17, 2019

Copy link
Copy Markdown
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 commented Jan 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

Will resume build.

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

Build resumed: https://ci.nodejs.org/job/node-test-pull-request/20186/

BridgeAR commented Jan 17, 2019

Copy link
Copy Markdown
Member

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

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

@BridgeAR thanks.

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
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 commented Jan 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

Ok resumed CI job is green :).

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

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

mhdawson commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

Landed in 1375af2

mhdawson added a commit that referenced this pull request Jan 17, 2019
test: revert fail coverage target if tests fail"
1375af2 
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 closed this Jan 17, 2019

refack commented Jan 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 commented Jan 17, 2019

Copy link
Copy Markdown
Contributor

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.

refack mentioned this pull request Jan 17, 2019
Closed
5 tasks
This was referenced Apr 23, 2019
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
mhdawson deleted the revertcov branch September 30, 2019 13:13
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL