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: fix bootstrap test #36418

Closed
wants to merge 1 commit into from
Closed

Conversation

benjamingr
Copy link
Member

@benjamingr benjamingr commented Dec 6, 2020

I landed PR 20de5f7 through NCU and somehow this change related to another PR landed in.

I'm investigating how that happened since NCU is supposed to validate a green CI of the code on GH

This PR removes the change. I'm investigating how that happened and am utterly confused.

Edit: #36308 (comment)

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

@benjamingr benjamingr added the test Issues and PRs related to the tests. label Dec 6, 2020
@benjamingr
Copy link
Member Author

👍 to fast track

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2020
@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/36418
✔  Done loading data for nodejs/node/pull/36418
----------------------------------- PR info ------------------------------------
Title      test: fix bootstrap test (#36418)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     benjamingr:fix-test -> nodejs:master
Labels     test
Commits    1
 - test: fix bootstrap test
Committers 1
 - Benjamin Gruenbaum 
PR-URL: https://github.com/nodejs/node/pull/36418
Reviewed-By: Gireesh Punathil 
Reviewed-By: Yongsheng Zhang 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36418
Reviewed-By: Gireesh Punathil 
Reviewed-By: Yongsheng Zhang 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-06T21:53:38Z: https://ci.nodejs.org/job/node-test-pull-request/34806/
- Querying data for job/node-test-pull-request/34806/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Sun, 06 Dec 2020 21:47:00 GMT
   ✔  Approvals: 2
   ✔  - Gireesh Punathil (@gireeshpunathil) (TSC): https://github.com/nodejs/node/pull/36418#pullrequestreview-545822681
   ✔  - Yongsheng Zhang (@ZYSzys): https://github.com/nodejs/node/pull/36418#pullrequestreview-545833789
   ✖  This PR needs to wait 38 more hours to land
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/405201532

@github-actions github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 7, 2020
@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. fast-track PRs that do not need to wait for 48 hours to land. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 7, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2020

Landed in ff048bd...537da19

@github-actions github-actions bot closed this Dec 7, 2020
nodejs-github-bot pushed a commit that referenced this pull request Dec 7, 2020
PR-URL: #36418
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@benjamingr benjamingr deleted the fix-test branch December 7, 2020 07:27
@Trott
Copy link
Member

Trott commented Dec 7, 2020

I'm investigating how that happened since NCU is supposed to validate a green CI of the code on GH

NCU only validates that the last CI is green and that no changes have landed in the PR since that CI. It will not validate that CI was run against the PR rebased against current master branch. That would be ideal, but I also imagine it would slow us waaaaay down. It probably isn't feasible until we can automate that commit queue more, which in turn isn't feasible until CI is more reliable.

@benjamingr
Copy link
Member Author

benjamingr commented Dec 7, 2020

Thanks @Trott that's interesting and indeed would be ideal.

I'm pretty sure this is my fault and not NCU's since after checking git reflow and my shell history what happened is:

  • I wanted to squash the commit and not go through CI again for no changes.
  • I pushed a change containing what I thought was a squash but somehow due to bad local state on my computer and a human error the change that wasn't supposed to be there.
  • I landed with NCU - ignoring the warning (since I thought I just squashed and didn't make any changes).

In retrospect I should have instead let NCU do the squash (which I assume is safer since it fetches the commits as a patch from the PR) instead of squashing manually beforehand. I am not sure why I didn't do that - I assume just because old bad habits die hard and I'm pretty inexperienced in landing things and re-learning.

I'm just happy I caught this because it caused a lot of damage and made a hotfix.

danielleadams pushed a commit that referenced this pull request Dec 7, 2020
PR-URL: #36418
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
PR-URL: nodejs#36418
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Yongsheng Zhang <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants