node-core-utils icon indicating copy to clipboard operation
node-core-utils copied to clipboard

feat: add auto run v8 ci

Open gengjiawen opened this issue 3 years ago • 19 comments

fix: https://github.com/nodejs/node-core-utils/issues/559 test pr: https://github.com/nodejs/node/pull/44986

Hopefully this will make us test v8 easier :)

local test

node bin/ncu-ci.js run 44986

currently I run into Jenkins credentials invalid. But I have used this credential before, and it worked.

cc @targos @richardlau @nodejs/v8-update

gengjiawen avatar Oct 13 '22 04:10 gengjiawen

$ git rev-parse --short HEAD
8e866ef
$ node bin/ncu-ci.js --owner nodejs --repo node run 44961
✔  Jenkins credentials valid
⠋ Starting PR CI job(node:1644671) [https://github.com/node-fetch/node-fetch/issues/1167] DeprecationWarning: form-data doesn't follow the spec and requires special treatment. Use alternative package
(Use `node --trace-deprecation ...` to show where the warning was created)
✔  PR CI job successfully started
✖  Failed to start CI
$

More info on the failure -- the "Failed to start CI" message is from the try-catch handler https://github.com/nodejs/node-core-utils/blob/8e866ef03f9ae21301024eaf22db87eb212a58b2/lib/ci/run_ci.js#L102 With additional debug:

diff --git a/lib/ci/run_ci.js b/lib/ci/run_ci.js
index e850a7a..51d6fb4 100644
--- a/lib/ci/run_ci.js
+++ b/lib/ci/run_ci.js
@@ -99,6 +99,7 @@ export class RunPRJob {
       }

     } catch (err) {
+      console.error(err);
       cli.stopSpinner('Failed to start CI', this.cli.SPINNER_STATUS.FAILED);
       return false;
     }

the error is:

$ node bin/ncu-ci.js --owner nodejs --repo node run 44986
✔  Jenkins credentials valid
✔  PR CI job successfully started
TypeError: Cannot read property 'gql' of undefined
    at PRData.getPR (file:///home/rlau/sandbox/github/node-core-utils/lib/pr_data.js:80:34)
    at RunPRJob.start (file:///home/rlau/sandbox/github/node-core-utils/lib/ci/run_ci.js:81:25)
    at processTicksAndRejections (internal/process/task_queues.js:95:5)
✖  Failed to start CI

richardlau avatar Oct 13 '22 11:10 richardlau

Is the credential put in ~/.ncurc

gengjiawen avatar Oct 13 '22 11:10 gengjiawen

Is the credential put in ~/.ncurc

Yes. Credential is working -- the pull request job was successfully started and it's the additions in this PR that are not working.

richardlau avatar Oct 13 '22 12:10 richardlau

Is the credential put in ~/.ncurc

Yes. Credential is working -- the pull request job was successfully started and it's the additions in this PR that are not working.

Yeap. On my local I stuck at credential issue. So I mark it as draft.

gengjiawen avatar Oct 13 '22 12:10 gengjiawen

@legendecas Is this because proxy issue, not supporting socks5 proxy ?

Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

gengjiawen avatar Oct 13 '22 12:10 gengjiawen

now jenkins with 500. Can you check the log @richardlau , thx.

gengjiawen avatar Oct 13 '22 12:10 gengjiawen

Now the issues is it's not auto-commenting pr ? which part should I check ?

gengjiawen avatar Oct 13 '22 12:10 gengjiawen

Looks good now (apart from the lint and test failures) and V8 CI is started:

$ node bin/ncu-ci.js --owner nodejs --repo node run 44986
✔  Jenkins credentials valid
⠋ Starting PR CI job(node:1665555) [https://github.com/node-fetch/node-fetch/issues/1167] DeprecationWarning: form-data doesn't follow the spec and requires special treatment. Use alternative package
(Use `node --trace-deprecation ...` to show where the warning was created)
✔  PR CI job successfully started
{ nodes: [ { name: 'v8 engine' } ] }
✔  V8 CI job successfully started
$

Started:

  • https://ci.nodejs.org/job/node-test-pull-request/47262/
  • https://ci.nodejs.org/job/node-test-commit-v8-linux/4912/

Refs: https://github.com/nodejs/node/pull/44986#issuecomment-1277653933

Now the issues is it's not auto-commenting pr ? which part should I check ?

This is done by the github-bot. I think this is the relevant code: https://github.com/nodejs/github-bot/blob/2f5b61f263ebeda2d9525733bb9dfa055fa24660/lib/push-jenkins-update.js#L15-L27

richardlau avatar Oct 13 '22 14:10 richardlau

test fixed.

please squash this commit for conventional commit.

gengjiawen avatar Oct 13 '22 15:10 gengjiawen

Codecov Report

Base: 84.03% // Head: 83.60% // Decreases project coverage by -0.42% :warning:

Coverage data is based on head (232b7d2) compared to base (4e59a64). Patch coverage: 35.00% of modified lines in pull request are covered.

:exclamation: Current head 232b7d2 differs from pull request most recent head 5fa57b9. Consider uploading reports for the commit 5fa57b9 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #652      +/-   ##
==========================================
- Coverage   84.03%   83.60%   -0.43%     
==========================================
  Files          37       37              
  Lines        4077     4117      +40     
==========================================
+ Hits         3426     3442      +16     
- Misses        651      675      +24     
Impacted Files Coverage Δ
lib/ci/run_ci.js 78.33% <35.00%> (-21.67%) :arrow_down:
lib/verbosity.js 80.76% <0.00%> (+7.69%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov[bot] avatar Oct 13 '22 15:10 codecov[bot]

@gengjiawen: @legendecas Is this because proxy issue, not supporting socks5 proxy ?

Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

Not sure what I can help here?

legendecas avatar Oct 13 '22 16:10 legendecas

@gengjiawen: @legendecas Is this because proxy issue, not supporting socks5 proxy ? Update: a China only issue, fucking gfw. proxy-agent looks only work in http proxy.

Not sure what I can help here?

fixed, proxy compatibility issue

gengjiawen avatar Oct 13 '22 16:10 gengjiawen

@joyeecheung Can you review this ?

gengjiawen avatar Oct 15 '22 11:10 gengjiawen

@aduh95 @nodejs/node-core-utils Can you take a look at this ? thx

gengjiawen avatar Oct 19 '22 09:10 gengjiawen

This looks good to me. Is there anything we can/should do about the small decrease in test coverage? I'm not worried about it if there isn't much to do.

Trott avatar Oct 19 '22 14:10 Trott

please squash this commit for conventional commit.

Do you want to squash and force push to your branch?

Trott avatar Oct 19 '22 14:10 Trott

please squash this commit for conventional commit.

Do you want to squash and force push to your branch?

I would like merger do this like we do in nodejs/node-gyp and nodejs/gyp-next.

gengjiawen avatar Oct 20 '22 01:10 gengjiawen

This looks good to me. Is there anything we can/should do about the small decrease in test coverage? I'm not worried about it if there isn't much to do.

It's a duplicate logic already tested (different URL and HTTP payload for v8 jenkins task). I think in this case, it's okay.

gengjiawen avatar Oct 20 '22 01:10 gengjiawen

@Trott Can you merge this and https://github.com/nodejs/github-bot/pull/353 (already reviewed by @joyeecheung ) ? thx

gengjiawen avatar Oct 21 '22 06:10 gengjiawen