-
-
Notifications
You must be signed in to change notification settings - Fork 34k
tests: fix inspector uri-encoded paths #59991
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
base: main
Are you sure you want to change the base?
Conversation
|
However I see that https://github.com/nodejs/node/actions/runs/17947125836/job/51036256456 So it's weird that it did fail previously. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59991 +/- ##
==========================================
- Coverage 88.45% 88.44% -0.02%
==========================================
Files 703 703
Lines 207544 207544
Branches 40013 40016 +3
==========================================
- Hits 183587 183563 -24
+ Misses 15965 15955 -10
- Partials 7992 8026 +34 🚀 New features to boost your workflow:
|
| session.post('Debugger.setBreakpointByUrl', { | ||
| 'lineNumber': 22, | ||
| 'url': pathToFileURL(path.resolve(__dirname, __filename)).toString(), | ||
| 'url': decodeURIComponent(pathToFileURL(path.resolve(__dirname, __filename)).toString()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct. The field url has to be a string in correct URL format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - I totally failed to fix this. I spent hours on this trying to find the root cause, no success.
My plan is to ignore that bug :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legendecas It seems that the inspector server can't resolve the file and fails to set breakpoints when the url has encoded values. Decoding the url by the client looks a valid workaround to me. What do you think?
Decoded URL
[inspector received] {"id":3,"method":"Debugger.setBreakpointByUrl","params":{"lineNumber":18,"url":"file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread~.js","columnNumber":0,"condition":""}}
[inspector send] {"id":3,"result":{"breakpointId":"1:18:0:file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread~.js","locations":[{"scriptId":"76","lineNumber":18,"columnNumber":8}]}}
Encoded URL
[inspector received] {"id":3,"method":"Debugger.setBreakpointByUrl","params":{"lineNumber":18,"url":"file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread%7E.js","columnNumber":0,"condition":""}}
[inspector send] {"id":3,"result":{"breakpointId":"1:18:0:file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread%7E.js","locations":[]}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there is already logic implemented to percent-encode special characters.
It's even tested by one of node's CI.
However, it fails for ~ (and + maybe), and this PR means nothing until it understands why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there is already logic implemented to percent-encode special characters.
Can you point it out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No ! I only know there is a test about it, and that test doesn't include ~ + chars
node/.github/workflows/test-linux.yml
Lines 72 to 78 in 3dba25f
| - name: Re-run test in a folder whose name contains unusual chars | |
| run: | | |
| mv node "$DIR" | |
| cd "$DIR" | |
| ./tools/test.py --flaky-tests keep_retrying -p actions -j 4 | |
| env: | |
| DIR: dir%20with $unusual"chars?'åß∂ƒ©∆¬…` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting point. The filename including ? can work fine.
[inspector received] {"id":3,"method":"Debugger.setBreakpointByUrl","params":{"lineNumber":18,"url":"file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread%3F.js","columnNumber":0,"condition":""}}
[inspector send] {"id":3,"result":{"breakpointId":"1:18:0:file:///Users/kohei/Documents/node/test/parallel/test-inspector-connect-main-thread%3F.js","locations":[{"scriptId":"76","lineNumber":18,"columnNumber":8}]}}
This fixes #59803