GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: improving code coverage for SourceMap class by italojs · Pull Request #43285 · nodejs/node

Conversation

@italojs

@italojs italojs commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

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

  • line 196

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

  • line 170-176

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

  • line 123-133 Captura de Tela 2022-06-01 às 12 22 38

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Jun 1, 2022
@aduh95
aduh95 requested a review from bcoe June 1, 2022 19:45
@bcoe

bcoe commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bcoe
bcoe approved these changes Jun 2, 2022
View reviewed changes
@italojs

italojs commented Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

@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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italojs

italojs commented Jun 3, 2022

Copy link
Copy Markdown
Contributor Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca
lpinca approved these changes Jun 3, 2022
View reviewed changes
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 3, 2022
@lpinca

lpinca commented Jun 3, 2022

Copy link
Copy Markdown
Member

@italojs "Test ASan" is flaky, don't worry about it.
italojs reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 3, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 3, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bcoe

bcoe commented Jun 5, 2022

Copy link
Copy Markdown
Contributor

@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@italojs
italojs force-pushed the test/source-map-improve-coverage branch from 5f30ecf to 056c660 Compare June 6, 2022 13:35
@nodejs-github-bot

nodejs-github-bot commented Jun 6, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca

lpinca commented Jun 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@italojs can you please remove the merge commits?
italojs reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Jun 12, 2022
View reviewed changes
@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. Only starts once the PR has an approving review. label Jun 12, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 12, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 12, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
CI Reliability 2022-06-13 nodejs/reliability#299
Open
15 tasks
@italojs

italojs commented Jun 13, 2022

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 13, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. 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
CI Reliability 2022-06-14 nodejs/reliability#300
Open
16 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
This was referenced Jun 15, 2022
CI Reliability 2022-06-15 nodejs/reliability#301
Open
CI Reliability 2022-06-16 nodejs/reliability#302
Open
CI Reliability 2022-06-17 nodejs/reliability#303
Open
@nodejs-github-bot

nodejs-github-bot commented Jun 20, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Jun 21, 2022
CI Reliability 2022-06-21 nodejs/reliability#307
Open
CI Reliability 2022-06-22 nodejs/reliability#308
Open
CI Reliability 2022-06-23 nodejs/reliability#309
Open
CI Reliability 2022-06-24 nodejs/reliability#310
Open
@italojs
italojs force-pushed the test/source-map-improve-coverage branch from 81a84d9 to 998acd4 Compare June 24, 2022 11:48
italojs added 2 commits June 24, 2022 09:00
@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
CI Reliability 2022-06-25 nodejs/reliability#311
Open
CI Reliability 2022-06-26 nodejs/reliability#312
Open
@italojs

italojs commented Jun 28, 2022

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 28, 2022
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Jun 28, 2022
@nodejs-github-bot

nodejs-github-bot commented Jun 28, 2022

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@lpinca

lpinca commented Jun 28, 2022

Copy link
Copy Markdown
Member

Landed in 6c1aa01.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca closed this Jun 28, 2022
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jul 12, 2022
v18.6.0 release proposal #43789
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Aug 3, 2022
v16.17.0 release proposal #44098
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 <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes
+1 more reviewer
@bcoe bcoe bcoe approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@italojs @bcoe @lpinca @nodejs-github-bot @jasnell
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.