FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: migrate message source map tests from Python to JS by yiyunlei · Pull Request #49238 · nodejs/node · GitHub

/ node Public

test: migrate message source map tests from Python to JS - #49238

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
yiyunlei:node47707-migrate-message-tests-source-map
Aug 29, 2023
ConversationCommits (1)ChecksFiles changed
Merged

test: migrate message source map tests from Python to JS#49238
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
yiyunlei:node47707-migrate-message-tests-source-map

Conversation

yiyunlei commented Aug 19, 2023

Copy link
Copy Markdown
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 added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Aug 19, 2023
yiyunlei marked this pull request as draft August 19, 2023 05:28
yiyunlei marked this pull request as ready for review August 19, 2023 05:28
yiyunlei mentioned this pull request Aug 19, 2023
Closed

MoLow commented Aug 19, 2023

Copy link
Copy Markdown
Member

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 commented Aug 22, 2023

Copy link
Copy Markdown
Contributor Author

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

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

MoLow commented Aug 23, 2023

Copy link
Copy Markdown
Member

commits still don't adhere guidelines

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
test: migrate message source map tests from Python to JS
0991cb3 
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 force-pushed the node47707-migrate-message-tests-source-map branch from e154b45 to 0991cb3 Compare August 24, 2023 16:42

yiyunlei commented Aug 24, 2023

Copy link
Copy Markdown
Contributor Author

I squashed my commits, hope it works this time.

MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 27, 2023
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 commented Aug 27, 2023

Copy link
Copy Markdown
Collaborator

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

github-actions Bot mentioned this pull request Aug 28, 2023
Open
25 tasks

yiyunlei commented Aug 28, 2023

Copy link
Copy Markdown
Contributor Author

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

github-actions Bot mentioned this pull request Aug 29, 2023
Open
21 tasks

nodejs-github-bot commented Aug 29, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow commented Aug 29, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

how can I pass the failed tests?

I ran a rebuild

nodejs-github-bot commented Aug 29, 2023

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 29, 2023

Copy link
Copy Markdown
Collaborator

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

MoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 29, 2023
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 merged commit 9cd70f4 into nodejs:main Aug 29, 2023

nodejs-github-bot commented Aug 29, 2023

Copy link
Copy Markdown
Collaborator

Landed in 9cd70f4

MoLow commented Aug 29, 2023

Copy link
Copy Markdown
Member

@yiyunlei thanks for your contribution!

yiyunlei commented Aug 29, 2023

Copy link
Copy Markdown
Contributor Author

@MoLow Thanks for your review!

This was referenced Aug 30, 2023
Open
Open
Open
Open
Open
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
test: migrate message source map tests from Python to JS
48e4156 
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 mentioned this pull request Sep 10, 2023
Merged
targos pushed a commit that referenced this pull request Nov 27, 2023
test: migrate message source map tests from Python to JS
107015b 
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>
targos mentioned this pull request Nov 28, 2023
Merged
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
test: migrate message source map tests from Python to JS
9c2c96a 
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
test: migrate message source map tests from Python to JS
cd5f216 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

MoLow MoLow approved these changes

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL