GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — test: fix parsing test flags by daeyeon · Pull Request #48012 · nodejs/node

Conversation

daeyeon commented May 15, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This does not replace _ with - when parsing flags.

I believe that it was added for a certain purpose, but it doesn't seem to be needed anymore. Currently, the cli options are not passed as intended in the following cases:

// Flags: --diagnostic-dir=~/PATH_HAS_UNDERSCORES

NOTE: The test started as a child_process using these flags: [ 
  '--diagnostic-dir=~/PATH-HAS-UNDERSCORES' <--
]

// Flags: --experimental-permission --allow-fs-read=~/PATH_HAS_UNDERSCORES

The test started as a child_process using these flags: [
  '--experimental-permission',
  '--allow-fs-read=~/PATH-HAS-UNDERSCORES'  <--
]

Signed-off-by: Daeyeon Jeong [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji

All reactions

👍 1 reaction


          test: fix parsing test flags

0c2e3a2

This removes replacing `_` with `-` in the flags defined.

Signed-off-by: Daeyeon Jeong <[email protected]>

nodejs-github-bot added needs-ci

PRs that need a full CI run.

test Issues and PRs related to Node.js core tests and test infrastructure. labels

May 15, 2023

MoLow approved these changes

May 15, 2023

View reviewed changes

MoLow left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I did not see any reasoning for this in the original PR adding this: #24876
and it matches the python behavior:

node/test/testpy/__init__.py

Lines 61 to 63 in a4e075f

	flags_match = FLAGS_PATTERN.search(source)
	if flags_match:
	flags = flags_match.group(1).strip().split()

so LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

daeyeon reacted with thumbs up emoji

All reactions

👍 1 reaction

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 15, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

May 15, 2023

This comment was marked as outdated.

nodejs-github-bot commented May 15, 2023

Copy link

Copy Markdown

Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51810/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

github-actions Bot mentioned this pull request

May 16, 2023

CI Reliability 2023-05-16 nodejs/reliability#567

Open

19 tasks

daeyeon added author ready

PRs with CI started, the required approvals, and no outstanding review comments.

commit-queue PRs queued for automated landing through the Commit Queue. labels

May 16, 2023

lpinca approved these changes

May 16, 2023

View reviewed changes

github-actions Bot mentioned this pull request

May 17, 2023

CI Reliability 2023-05-17 nodejs/reliability#568

Open

17 tasks

nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label

May 17, 2023

nodejs-github-bot merged commit 5cb5422 into nodejs:main

May 17, 2023

nodejs-github-bot commented May 17, 2023

Copy link

Copy Markdown

Collaborator

Landed in 5cb5422

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 18, 2023

CI Reliability 2023-05-18 nodejs/reliability#569

Open

CI Reliability 2023-05-19 nodejs/reliability#570

Open

CI Reliability 2023-05-20 nodejs/reliability#571

Open

CI Reliability 2023-05-21 nodejs/reliability#572

Open

targos pushed a commit that referenced this pull request

May 30, 2023


          test: fix parsing test flags

a41cfd1

This removes replacing `_` with `-` in the flags defined.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #48012
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

targos mentioned this pull request

Jun 4, 2023

v20.3.0 release proposal #48332

Merged

danielleadams pushed a commit that referenced this pull request

Jul 6, 2023


          test: fix parsing test flags

bef81f4

This removes replacing `_` with `-` in the flags defined.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: #48012
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

MoLow pushed a commit to MoLow/node that referenced this pull request

Jul 6, 2023


          test: fix parsing test flags

ca4a0e3

This removes replacing `_` with `-` in the flags defined.

Signed-off-by: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#48012
Reviewed-By: Moshe Atlow <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>

danielleadams mentioned this pull request

Jul 10, 2023

v18.17.0 release proposal #48694

Merged

Reviewers

lpinca

lpinca approved these changes

MoLow

MoLow approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

needs-ci

PRs that need a full CI run.

test

Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

daeyeon commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented May 15, 2023

Uh oh!

nodejs-github-bot commented May 17, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

daeyeon commented May 15, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.