Skip to content

test: Migrate message error tests from Python to JS - #49738

Open
akhateeb22 wants to merge 3 commits into
nodejs:mainfrom
akhateeb22:actions/migrate-tests
Open

test: Migrate message error tests from Python to JS#49738
akhateeb22 wants to merge 3 commits into
nodejs:mainfrom
akhateeb22:actions/migrate-tests

Conversation

@akhateeb22

@akhateeb22akhateeb22 commented Sep 20, 2023

Copy link
Copy Markdown

No description provided.

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Sep 20, 2023
@akhateeb22akhateeb22 mentioned this pull request Sep 20, 2023
Closed
MoLow
MoLow reviewed Sep 20, 2023
View reviewed changes
Comment threadtest/parallel/test-output-assert.mjs Outdated
MoLow
MoLow reviewed Sep 20, 2023
View reviewed changes

@MoLowMoLow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, minor fix needed

@akhateeb22

akhateeb22 commented Sep 20, 2023

Copy link
Copy Markdown
Author

@MoLow, Committed, Thank you

@MoLowMoLow added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 20, 2023
@akhateeb22

akhateeb22 commented Sep 20, 2023

Copy link
Copy Markdown
Author

@MoLow Hello, Thanks for reviewing, I committed some changes

atlowChemi
atlowChemi reviewed Sep 20, 2023
View reviewed changes
Comment threadpackage-lock.json Outdated
atlowChemi
atlowChemi reviewed Sep 20, 2023
View reviewed changes
Comment threadtest/parallel/test-output-assert.mjs Outdated
atlowChemi
atlowChemi reviewed Sep 20, 2023
View reviewed changes
Comment threadtest/fixtures/assert/assert_throws_stack.snapshot
Comment on lines -22 to -25
at assert.throws.bar (*assert_throws_stack.js:*)
at getActual (node:assert:*)
at Function.throws (node:assert:*)
at Object.<anonymous> (*assert_throws_stack.js:*:*)

@atlowChemiatlowChemiSep 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't the test checking exactly for this stack?

@akhateeb22akhateeb22Sep 21, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

the test should check for it exactly ?, current test generates:

 at * {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: Error: foo
at *
at *
at *
at *
at *
at *
at *
at *
at *
at node:internal*main*run_main_module**,
expected: { bar: true },
operator: 'throws'
}

@MoLowMoLowSep 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is the original PR adding this test: #27243
@BridgeAR is this test intended to snapshot the entire stack trace? is this change ok?

@akhateeb22akhateeb22Sep 22, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Any idea or suggestions to close this PR ?

@MoLowMoLowSep 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe add another trasnform function to make this snapshot similar to the original one?

@aduh95

aduh95 commented May 11, 2024

Copy link
Copy Markdown
Contributor

This needs a rebase to solve the git conflicts.

@github-actions

github-actionsBot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MoLowMoLowMoLow approved these changes
@atlowChemiatlowChemiatlowChemi left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.staletestIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@akhateeb22@aduh95@MoLow@atlowChemi@nodejs-github-bot