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 message source map tests from Python to JS #49238

Conversation

yiyunlei
Copy link
Contributor

Some JS files are detected as deleted and newly created even though they are very similar. I also mentioned the reason here #47707 (comment). IMHO, it is because the default similarity index for git diff is 50%.

Same for most .snapshot files ( they are also detected as deleted ), the main difference between the original .out file and the .snapshot file is .snapshot files have more details like Object.<anonymous>.

Example:

  • node/test/fixtures/source-map/output/source_map_throw_catch.snapshot
reachable
Error: an exception
    at branch (*typescript-throw.ts:18:11)
    at Object.<anonymous> (*typescript-throw.ts:24:1)
  • node/test/message/source_map_throw_catch.out
reachable
Error: an exception
    at *typescript-throw.ts:18:11*
    at *typescript-throw.ts:24:1*

Should I replace these Object.<anonymous>?

Also, #47707 (comment)

In the latest code version, the tests for source_map_disabled_by_api.js and source_map_enabled_by_api.js are failing because of the change in the commit #46391.

Should I update the .snapshot files for them accordingly ?

@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 Aug 19, 2023
@yiyunlei yiyunlei marked this pull request as draft August 19, 2023 05:28
@yiyunlei yiyunlei marked this pull request as ready for review August 19, 2023 05:28
@MoLow
Copy link
Member

MoLow commented Aug 19, 2023

Thanks for this work! I don't think we need to make such an effort for git to detect these as renames as long as reviewing these PRs makes sense and doesn't include hundreds of files :)

also, please fix lint errors and amend the commit message for it to comply with guidlines

@yiyunlei
Copy link
Contributor Author

Thanks for your review. I have updated the commit message and the code according to Lint.

@yiyunlei yiyunlei requested a review from MoLow August 22, 2023 17:58
@yiyunlei yiyunlei force-pushed the node47707-migrate-message-tests-source-map branch from 46d42ca to bee982b Compare August 22, 2023 23:15
@MoLow
Copy link
Member

MoLow commented Aug 23, 2023

commits still don't adhere guidelines

@yiyunlei yiyunlei force-pushed the node47707-migrate-message-tests-source-map branch 2 times, most recently from 0a273de to e154b45 Compare August 24, 2023 16:36
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: nodejs#47707

test: migrate message source map tests from Python to JS
@yiyunlei yiyunlei force-pushed the node47707-migrate-message-tests-source-map branch from e154b45 to 0991cb3 Compare August 24, 2023 16:42
@yiyunlei
Copy link
Contributor Author

I squashed my commits, hope it works this time.

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

@yiyunlei
Copy link
Contributor Author

Hi @MoLow, how can I pass the failed tests?

@nodejs-github-bot
Copy link
Collaborator

@MoLow
Copy link
Member

MoLow commented Aug 29, 2023

how can I pass the failed tests?

I ran a rebuild

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@MoLow MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2023
@nodejs-github-bot nodejs-github-bot merged commit 9cd70f4 into nodejs:main Aug 29, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 9cd70f4

@MoLow
Copy link
Member

MoLow commented Aug 29, 2023

@yiyunlei thanks for your contribution!

@yiyunlei
Copy link
Contributor Author

@MoLow Thanks for your review!

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: #47707

test: migrate message source map tests from Python to JS
PR-URL: #49238
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: nodejs#47707

test: migrate message source map tests from Python to JS
PR-URL: nodejs#49238
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this pull request Nov 27, 2023
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: #47707

test: migrate message source map tests from Python to JS
PR-URL: #49238
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: nodejs/node#47707

test: migrate message source map tests from Python to JS
PR-URL: nodejs/node#49238
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Migrate the remaining source map tests in the `test/message` folder
from Python to JS.

Fixes: nodejs/node#47707

test: migrate message source map tests from Python to JS
PR-URL: nodejs/node#49238
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
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.

3 participants