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: improving code coverage for SourceMap class #43285

Closed
wants to merge 2 commits into from
Closed

test: improving code coverage for SourceMap class #43285

wants to merge 2 commits into from

Conversation

italojs
Copy link
Contributor

@italojs italojs commented Jun 1, 2022

Improve code coverage for SourceMap class

refs:
I noted we don't cover the malformed mapping scenario like this or this

so I would like to cover this lines

Captura de Tela 2022-06-01 às 12 23 48

Captura de Tela 2022-06-01 às 12 25 46

@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 Jun 1, 2022
@aduh95 aduh95 requested a review from bcoe June 1, 2022 19:45
@bcoe
Copy link
Contributor

bcoe commented Jun 2, 2022

@italojs looks reasonable to me, can you provide a before and after example of the lines that this test covers specifically? Is this what the images you've provided represents (I'm surprised findEntry or new SourceMap wasn't covered prior to this).

@italojs
Copy link
Contributor Author

italojs commented Jun 3, 2022

@bcoe you could find it in my screenshots, the code at the left side is the coverage after my tests, and the code on the right side is the coverage before my tests

@italojs
Copy link
Contributor Author

italojs commented Jun 3, 2022

@bcoe my test ASan is getting failed(timeout), I didn't find anything in the node documentation about this test to run it locally or rerun it in gh actions
could you give some instructions to solve it, please?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2022
@lpinca
Copy link
Member

lpinca commented Jun 3, 2022

@italojs "Test ASan" is flaky, don't worry about it.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@bcoe
Copy link
Contributor

bcoe commented Jun 5, 2022

@italojs could I bother you to rebase your PR against head, looks like tests failed due to a merge conflict.

I'll kick off tests as soon as you've pushed.

@italojs italojs force-pushed the test/source-map-improve-coverage branch from 5f30ecf to 056c660 Compare June 6, 2022 13:35
@nodejs-github-bot
Copy link
Collaborator

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

@lpinca
Copy link
Member

lpinca commented Jun 11, 2022 •
edited
Loading

@italojs can you please remove the merge commits?

@italojs italojs force-pushed the test/source-map-improve-coverage branch from 9034d8e to 391c2df Compare June 12, 2022 16:07
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 12, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@italojs italojs force-pushed the test/source-map-improve-coverage branch from 391c2df to 998acd4 Compare June 12, 2022 21:25
@github-actions github-actions bot mentioned this pull request Jun 13, 2022
Open
15 tasks
@italojs
Copy link
Contributor Author

italojs commented Jun 13, 2022

@bcoe @jasnell @lpinca I think everything is passing now, have some additional steps before we merge it?

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 13, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Jun 14, 2022
Open
16 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Jun 15, 2022
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 21, 2022
Open
Open
Open
Open
@italojs italojs force-pushed the test/source-map-improve-coverage branch from 81a84d9 to 998acd4 Compare June 24, 2022 11:48
@italojs
test: coverage malformed mapping
e673ab3 
covering malformed mappings scenario, sourceMap class
@italojs
test: fixing lint
0e4364c 
fix lint
@italojs italojs force-pushed the test/source-map-improve-coverage branch from 998acd4 to 0e4364c Compare June 24, 2022 12:01
This was referenced Jun 25, 2022
Open
Open
@italojs
Copy link
Contributor Author

italojs commented Jun 28, 2022

Hi @lpinca
as you said the "Test ASan" is flaky, do we merge with this test failing, and we run it until it works... what do you suggest?? this is the unique point to finalize this PR

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

lpinca pushed a commit that referenced this pull request Jun 28, 2022
@italojs @lpinca
test: improve code coverage for SourceMap class
6c1aa01 
Cover malformed mappings scenario.

PR-URL: #43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca
Copy link
Member

lpinca commented Jun 28, 2022

Landed in 6c1aa01.

@lpinca lpinca closed this Jun 28, 2022
mabaasit pushed a commit to mabaasit/node that referenced this pull request Jul 6, 2022
@italojs @mabaasit
test: improve code coverage for SourceMap class
f9dc9aa 
Cover malformed mappings scenario.

PR-URL: nodejs#43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
@italojs @targos
test: improve code coverage for SourceMap class
705e85e 
Cover malformed mappings scenario.

PR-URL: #43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
targos pushed a commit that referenced this pull request Jul 20, 2022
@italojs @targos
test: improve code coverage for SourceMap class
bb81950 
Cover malformed mappings scenario.

PR-URL: #43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
@italojs @targos
test: improve code coverage for SourceMap class
e8754f3 
Cover malformed mappings scenario.

PR-URL: #43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@italojs @guangwong
test: improve code coverage for SourceMap class
34fc59f 
Cover malformed mappings scenario.

PR-URL: nodejs/node#43285
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcoe bcoe bcoe approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca 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.
5 participants
@italojs @bcoe @lpinca @nodejs-github-bot @jasnell