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 zlib version regex #48227

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented May 28, 2023

Add support for subrevision in the regular expression for the zlib version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 28, 2023
@lpinca lpinca force-pushed the fix/version-regex branch from dce2065 to 641b4db Compare May 28, 2023 06:05
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@lpinca
Copy link
Member Author

lpinca commented May 28, 2023

Fixes CI for #48218.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@@ -61,7 +61,7 @@ assert.match(process.versions.brotli, commonTemplate);
assert.match(process.versions.llhttp, commonTemplate);
assert.match(process.versions.node, commonTemplate);
assert.match(process.versions.uv, commonTemplate);
assert.match(process.versions.zlib, commonTemplate);
assert.match(process.versions.zlib, /^\d+\.\d+\.\d+(?:\.\d+)?(?:-.*)?$/);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert.match(process.versions.zlib, /^\d+\.\d+\.\d+(?:\.\d+)?(?:-.*)?$/);
assert.match(process.versions.zlib, /^\d+(?:\.\d+){2,3}(?:-.*)?$/);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency I would prefer to keep it as is, see line 72. I'm ok with a a new PR to shorten both regex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, the reason the first regex wasn't shortened that way is that it would complicate it more than anything else. Unlike this one, which is already a little longer, and in my opinion my suggestion is just as consistent as this regex. But I understand and I don't mind a new PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm talking about this one

/^\d+\.\d+\.\d+(?:\.\d+)?-node\.\d+(?: \(candidate\))?$/
but I have no strong opinion. I will apply your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're completly right, sorry for my misunderstanding.

Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
@lpinca lpinca force-pushed the fix/version-regex branch from 641b4db to 43c332f Compare May 28, 2023 18:32
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label May 29, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 30, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6a6b3c5 into nodejs:main May 30, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6a6b3c5

targos pushed a commit that referenced this pull request May 30, 2023
Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
PR-URL: #48227
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca lpinca deleted the fix/version-regex branch May 30, 2023 13:31
@targos targos mentioned this pull request Jun 4, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
PR-URL: #48227
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
PR-URL: nodejs#48227
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
PR-URL: nodejs#48227
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Add support for subrevision in the regular expression for the zlib
version.

Refs: https://github.com/madler/zlib/blob/48c3741002ac/zlib.h#L40
PR-URL: nodejs#48227
Reviewed-By: Mestery <mestery@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants