nodejs / llnode

An lldb plugin for Node.js and V8, which enables inspection of JavaScript states for insights into Node.js processes and their core dumps.
Other
1.15k stars 99 forks source link

Upgrade GitHub Actions #403

Closed cclauss closed 1 year ago

cclauss commented 1 year ago
codecov-commenter commented 1 year ago

Codecov Report

Merging #403 (aa5ecf4) into main (ff75da7) will decrease coverage by 0.37%. The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #403      +/-   ##
==========================================
- Coverage   77.52%   77.14%   -0.38%     
==========================================
  Files          34       34              
  Lines        5027     5027              
==========================================
- Hits         3897     3878      -19     
- Misses       1130     1149      +19     
Impacted Files Coverage Δ
src/llscan.cc 60.56% <0.00%> (-1.86%) :arrow_down:
src/llv8.cc 71.67% <0.00%> (-0.27%) :arrow_down:

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

No9 commented 1 year ago

Hey @cclauss thanks for the PR but this work is already covered under these two PRs llvm - https://github.com/nodejs/llnode/pull/389 16/18 upgrade https://github.com/nodejs/llnode/issues/399 If you're trying for something else then please let us know or we can close this PR.

cclauss commented 1 year ago

Let's merely upgrade the GitHub Actions.

No9 commented 1 year ago

Excellent - The bump of actions, especially to the official setup-node, is very useful. As this could impact #389 from both a merge and feature perspective I'm going to land that first then merge this. Current plan is to do the merge of 389 by the end of this week - 2022-08-21.

Edit: I thought this fixed https://github.com/cclauss/llnode/blob/6ac110d4f6fea7b898b01e4b67669d502b15c07d/.github/workflows/push.yml#L35 But it looks like this PR needs to be updated https://github.com/actions/setup-node/pull/360 I still think it would be good to land this but the dep on my setup-node needs to be fixed.

No9 commented 1 year ago

Hey @cclauss

389 has merged - If you could rebase this I would be happy to help land

cclauss commented 1 year ago

Rebased. Also fixed runs-on: to take a string, not an array because the runner is not self-hosted.

No9 commented 1 year ago

Hey @cclauss There seems to be a tests that are on the edge of the timeouts for node14 with lldb12

Would you mind bumping https://github.com/nodejs/llnode/blob/ff75da7eb151cf8d7481eba9a1c6406a53c85ce5/test/plugin/inspect-test.js#L669 to 30000 and https://github.com/nodejs/llnode/blob/ff75da7eb151cf8d7481eba9a1c6406a53c85ce5/test/plugin/workqueue-test.js#L29 to 60000 And set the defaults to 40000 https://github.com/nodejs/llnode/blob/ff75da7eb151cf8d7481eba9a1c6406a53c85ce5/test/common.js#L45 https://github.com/nodejs/llnode/blob/ff75da7eb151cf8d7481eba9a1c6406a53c85ce5/test/common.js#L173

Thanks

cclauss commented 1 year ago

@No9 Your re-review, please.

No9 commented 1 year ago

Hey @cclauss I think these have landed in 404 as you requested https://github.com/nodejs/llnode/pull/404/files#diff-f3fc934cf0d89bdf07de358896ff90f1202585df812cf606206d1830a9949811R37

cclauss commented 1 year ago

Closing in favor of #404