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.

Sign up for GitHub

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

Jump to bottom

test: migrate messages v8 tests from Python to JS #51534

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test: migrate messages v8 tests from Python to JS #51534

wants to merge 1 commit into from

Conversation

ognjenjevremovic
Copy link
Contributor

@ognjenjevremovic ognjenjevremovic commented Jan 20, 2024 •
edited
Loading

Migrated messages v8 tests from Python to JavaScript. This change is part of an effort to enhance maintainability and consistency in the test suite. All relevant test files and dependencies have been updated accordingly.

Refs: #47707

Summary

This pull request addresses Issue #47707, which involved migrating messages v8 tests from Python to JavaScript to enhance maintainability and consistency in the test suite.

Changes Made

  • Migrated messages v8 tests from Python to JavaScript.
  • Updated all relevant test files and snapshots accordingly.

Additional Notes

  • The changes adhere to the Node.js testing and coding style guidelines.
  • Automated tests for the migrated messages v8 tests have passed successfully.

Tests affected:

- errors/assert_throws_stack.js
- errors/console_assert.js
- errors/eval_messages.js
- errors/internal_assert.js
- errors/internal_assert_fail.js

Related Issues

Improves: Issue #47707

Checklist

  • Test changes have been linted and pass the automated tests.
  • Code follows the Node.js testing and coding style guide.
  • Commits are atomic and convey meaningful changes.
  • Pull request is based on the latest main branch.

@ognjenjevremovic
test: migrate messages v8 tests from Python to JS
bec5115 
Migrated messages v8 tests from Python to JavaScript. This change is
part of an effort to enhance maintainability and consistency in the
test suite. All relevant test files and dependencies have been updated
accordingly.

Refs: nodejs#47707
@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 Jan 20, 2024
@lemire lemire added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 26, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 26, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@ognjenjevremovic
Copy link
Contributor Author

Hey just wondering, are the failing tests actually related to changes introduced in this PR?
Is there any additional steps (or fixes) I should provide regarding this PR?

GeoffreyBooth
GeoffreyBooth reviewed Mar 23, 2024
View reviewed changes
test/fixtures/errors/assert_throws_stack.snapshot
Comment on lines +12 to +18
at Object.<anonymous> (*assert_throws_stack.js:*:*)
at Module._compile (node:internal*modules*cjs*loader:*:*)
at Module._extensions..js (node:internal*modules*cjs*loader:*:*)
at Module.load (node:internal*modules*cjs*loader:*:*)
at Module._load (node:internal*modules*cjs*loader:*:*)
at Function.executeUserEntryPoint [as runMain] (node:internal*modules*run_main:*:*)
at node:internal*main*run_main_module:*:* {
Copy link
Member

Choose a reason for hiding this comment

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

We need to get all of this out of the snapshot, so that changes to the CommonJS loader don’t break tests such as this one. You can use Error.stackTraceLimit = 1 or similar for this (see the similar tests nearby).

test/fixtures/errors/eval_messages.snapshot
Comment on lines +7 to +12
at makeContextifyScript (node:internal*vm:*:*)
at node:internal*process*execution:*:*
at [eval]-wrapper:*:*
at runScript (node:internal*process*execution:*:*)
at evalScript (node:internal*process*execution:*:*)
at node:internal*main*eval_string:*:*
Copy link
Member

Choose a reason for hiding this comment

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

Same here and below.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Mar 23, 2024 •
edited
Loading

Hey just wondering, are the failing tests actually related to changes introduced in this PR?

Probably not, especially if they happen only on certain platforms. We generally click “resume” a few times to rerun CI because many tests are flaky.

Sorry for not getting to this sooner! Thank you for the contribution!

@bjohansebas bjohansebas added the stalled Issues and PRs that are stalled. label Apr 6, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 6, 2025

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

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

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

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

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ognjenjevremovic @nodejs-github-bot @GeoffreyBooth @lemire @bjohansebas