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: migrate messages v8 tests from Python to JS #51534

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

Conversation

ognjenjevremovic
Copy link
Contributor

@ognjenjevremovic ognjenjevremovic commented Jan 20, 2024

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.

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?

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).

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

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!

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