node icon indicating copy to clipboard operation
node copied to clipboard

tools: add WPT updater for specific subsystems

Open mertcanaltin opened this issue 1 year ago • 27 comments

a WPT updater that specifically targets the url subsystem for more controlled and focused updates.

mertcanaltin avatar Aug 20 '24 07:08 mertcanaltin

Review requested:

  • [ ] @nodejs/actions

nodejs-github-bot avatar Aug 20 '24 07:08 nodejs-github-bot

thanks for your suggestions

mertcanaltin avatar Aug 20 '24 12:08 mertcanaltin

By the way, I'm trying to update the WPT files in #54468.

IMO this could be done via

git node wpt <...>

Because the README.md file also needs to be updated.

avivkeller avatar Aug 23 '24 11:08 avivkeller

By the way, I'm trying to update the WPT files in #54468.

IMO this could be done via

git node wpt <...>

Because the README.md file also needs to be updated.

Do you have a suggestion, would that be enough for this?

- Name: Update README.md file
        Run: |
            # Update the README.md file with the new WPT version
            # Assuming you have a script or command to do this
            ./scripts/update-readme.sh ‘${{{ env.NEW_VERSION }}’

      - Name: Commit to Changes
        Run: |
            git config --global user.name ‘Node.js GitHub Bot’
            git config --global user.email ‘[email protected]’
            git add test/fixtures/wpt README.md
            git commit -m ‘test: Update WPT to ${{{ env.NEW_VERSION }} and update README.md’

mertcanaltin avatar Aug 23 '24 17:08 mertcanaltin

In my opinion, the entire WPT update process could be moved to a dep-updater script, using git node for each section. But, there are a lot of cases where updating these tests cause problems (just look at the errors in my PR), so maybe keeping it manual is good?

I'm not sure.

avivkeller avatar Aug 23 '24 17:08 avivkeller

cc @nodejs/web-standards

panva avatar Aug 26 '24 13:08 panva

yeah, we had discussion at https://github.com/nodejs/node/pull/50567.

legendecas avatar Aug 28 '24 10:08 legendecas

I'm closing this place, thank you very much for the comments

mertcanaltin avatar Sep 17 '24 06:09 mertcanaltin

@panva What do you think about adding a WPT updater but for specific tests? Ada repository has a WPT updater for url and it works perfectly. I'd be interested if we have it for url, and I'm more than happy to intervene and fix the pull-request in case something breaks.

anonrig avatar Sep 17 '24 14:09 anonrig

@anonrig That's pretty much a version of what I think we should be doing, only scoped to a particular WPT suite.

In my opinion a sensible way to update WPTs would be to pull a WPT subsystem from upstream (using git node wpt ...), run it, if it passes - open a PR, if it fails - have a keepalive issue where pending WPT updates are tracked.

panva avatar Sep 17 '24 14:09 panva

Perfect @mertcanaltin seems interested in pursuing this. Mertcan can you add a WPT updater but only for url? Make sure to have a list for the scopes, so we can add them depending on people's interest.

anonrig avatar Sep 17 '24 14:09 anonrig

This looks great, I'm gonna do it

mertcanaltin avatar Sep 17 '24 14:09 mertcanaltin

Will you test this in https://github.com/nodejs/node-auto-test?

panva avatar Sep 21 '24 11:09 panva

https://github.com/mertcanaltin/node-auto-test/actions/runs/10972255885/job/30468306292 I did a test run here and it seems to give an error to the github token

mertcanaltin avatar Sep 21 '24 11:09 mertcanaltin

mertcanaltin/node-auto-test/actions/runs/10972255885/job/30468306292 I did a test run here and it seems to give an error to the github token

I think it seems to suggest the script is ending prematurely. i.e. not syntactically correct.

panva avatar Sep 21 '24 11:09 panva

Passed 🎉 https://github.com/mertcanaltin/node-auto-test/actions/runs/10972357345/job/30468531837

The issue was that the if block wasn’t properly closed. I just added the missing fi at the end, and now everything works smoothly.

mertcanaltin avatar Sep 21 '24 12:09 mertcanaltin

Passed 🎉 mertcanaltin/node-auto-test/actions/runs/10972357345/job/30468531837

The issue was that the if block wasn’t properly closed. I just added the missing fi at the end, and now everything works smoothly.

Can you run dispatch with two subsystems? url and WebCryptoAPI. The latter should open a PR.

panva avatar Sep 21 '24 12:09 panva

https://github.com/mertcanaltin/node-auto-test/actions/runs/10972538059 I ran for both of them

mertcanaltin avatar Sep 21 '24 12:09 mertcanaltin

https://github.com/mertcanaltin/node-auto-test/actions/runs/10972538059/job/30468925665#step:4:7

It would appear @node-core/utils needs its config file with credentials to run this. Maybe an empty config file would do? I dunno, I'm short on time to look into this.

panva avatar Sep 21 '24 12:09 panva

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

avivkeller avatar Sep 21 '24 12:09 avivkeller

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

ı applied the latest updates, thank you

mertcanaltin avatar Sep 21 '24 12:09 mertcanaltin

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

node/.github/workflows/update-openssl.yml

I looked here as an example

mertcanaltin avatar Sep 21 '24 12:09 mertcanaltin

Look at .github/workflows/auto-start-ci.yml for Setup @node-core/utils

panva avatar Sep 21 '24 13:09 panva

I believe the last 2 points weren't addressed. ncu is not configured and the new version variable is used before it is set

panva avatar Sep 28 '24 07:09 panva

I believe the last 2 points weren't addressed. ncu is not configured and the new version variable is used before it is set

Thanks a lot for your detailed feedback! I’ve now addressed the last two points you mentioned:

I've added a step to configure ncu (node-core-utils) with the necessary GitHub token. I moved the calculation of the new_version earlier in the workflow to ensure it's set before being referenced.

mertcanaltin avatar Sep 28 '24 09:09 mertcanaltin

@nodejs/actions please review

panva avatar Sep 28 '24 11:09 panva

~~FYI I was looking at PRs and I saw #50567 cc @legendecas~~ I'm really late to the party, sorry

https://github.com/nodejs/node/pull/54460#issuecomment-2314933326

avivkeller avatar Oct 17 '24 18:10 avivkeller

Landed in 4f62ab5af8e98c73dcb56399fa913bf52e0c0c01

nodejs-github-bot avatar Nov 26 '24 13:11 nodejs-github-bot

🎉

https://github.com/nodejs/node/actions/runs/12031931489 https://github.com/nodejs/node/pull/55999

More subsystems can be added like so

panva avatar Nov 26 '24 13:11 panva