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: check inspector command Debugger.setInstrumentationBreakpoint #31137

Merged
merged 6 commits into from
May 12, 2024
Merged

test: check inspector command Debugger.setInstrumentationBreakpoint #31137

merged 6 commits into from
May 12, 2024

Conversation

ulitink
Copy link
Contributor

@ulitink ulitink commented Dec 30, 2019 •
edited
Loading

Adds test case for inspector's method Debugger.setInstrumentationBreakpoint with beforeScriptExecution param.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 30, 2019
@ulitink ulitink mentioned this pull request Dec 30, 2019
Open
@addaleax addaleax added the inspector Issues and PRs related to the V8 inspector protocol label Dec 30, 2019
@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2020

@ulitink is this ready to review? Or is this still work in progress?

@ulitink
Copy link
Contributor Author

ulitink commented Jan 5, 2020

@BridgeAR It is a test for an issue #31138. We can't merge this pull request until the issue is fixed because the test is broken. Sorry if such workflow is bad, we can simply reject the pull request and wait for the issue to be fixed, test is still referenced from it and can be found.

@BridgeAR
Copy link
Member

BridgeAR commented Jan 6, 2020

It is possible to add this test and mark it as failing in our status file. That way it would only fail in case this test passes. If I understood correct, this test is legit and it should pass while it fails at the moment. That way it's possible to just remove the test from our status file while fixing the issue.

@Trott
Copy link
Member

Trott commented Jan 6, 2020

It is possible to add this test and mark it as failing in our status file. That way it would only fail in case this test passes. If I understood correct, this test is legit and it should pass while it fails at the moment. That way it's possible to just remove the test from our status file while fixing the issue.

If the test fails 100% of the time, probably best to put it in known_issues and then we can move it out to parallel (or wherever) when the fix is committed.

@ulitink ulitink marked this pull request as ready for review January 11, 2020 03:05
BridgeAR
BridgeAR approved these changes Jan 11, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Jan 14, 2020

@nodejs/inspector

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@gireeshpunathil gireeshpunathil added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020
@gireeshpunathil
Copy link
Member

Is this ready to land?

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 5, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 closed this Sep 18, 2023
@aduh95 aduh95 reopened this Sep 18, 2023
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 18, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Oct 10, 2023
Open
24 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 force-pushed the inspector-instrumentation-breakpoint branch from ec2b8e9 to c6d069d Compare October 23, 2023 18:21
@nodejs-github-bot
Copy link
Collaborator

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

@larson-carter
Copy link
Member

This is a quite old PR, I see it ties into #31138 is there any updates?

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 reviewed May 11, 2024
View reviewed changes
test/known_issues/test-inspector-instrumentation-breakpoint.js
'use strict';
const common = require('../common');

common.skipIfInspectorDisabled();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like skipping makes the test succeeds, which is considered a failure for known_issues/ tests..

@aduh95 aduh95 force-pushed the inspector-instrumentation-breakpoint branch from c6d069d to 29607a1 Compare May 11, 2024 08:42
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@aduh95 aduh95 merged commit 291c121 into nodejs:main May 12, 2024
50 checks passed
@aduh95
Copy link
Contributor

aduh95 commented May 12, 2024

Landed in 291c121

targos pushed a commit that referenced this pull request May 13, 2024
@ulitink @targos
test: add Debugger.setInstrumentationBreakpoint known issue
e016e95 
PR-URL: #31137
Refs: #31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request May 13, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@ulitink @marco-ippolito
test: add Debugger.setInstrumentationBreakpoint known issue
7ce8733 
PR-URL: #31137
Refs: #31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Jun 17, 2024
Merged
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@ulitink @marco-ippolito
test: add Debugger.setInstrumentationBreakpoint known issue
a536d0d 
PR-URL: #31137
Refs: #31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@ulitink @marco-ippolito
test: add Debugger.setInstrumentationBreakpoint known issue
65fa95d 
PR-URL: #31137
Refs: #31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
sophonieb pushed a commit to sophonieb/node that referenced this pull request Jun 20, 2024
@ulitink @sophonieb
test: add Debugger.setInstrumentationBreakpoint known issue
26bd936 
PR-URL: nodejs#31137
Refs: nodejs#31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@ulitink @bmeck
test: add Debugger.setInstrumentationBreakpoint known issue
2ed8678 
PR-URL: nodejs#31137
Refs: nodejs#31138
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[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

@aduh95 aduh95 aduh95 left review comments

@Trott Trott Trott approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@ulitink @BridgeAR @Trott @nodejs-github-bot @gireeshpunathil @larson-carter @aduh95 @jasnell @addaleax