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.

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

test: increase coverage of pathToFileURL #55493

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 22, 2024

In #55476, we're changing the implementation of pathToFileURL to something more performant. In an early implementation, I realized some char were swallowed, but we didn't have a test to catch that. This PR is adding some additional test cases to catch this kind of bug.

@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 Oct 22, 2024
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.41%. Comparing base (a3693d2) to head (2753b70).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55493   +/-   ##
=======================================
  Coverage   88.41%   88.41%           
=======================================
  Files         653      653           
  Lines      187437   187437           
  Branches    36074    36078    +4     
=======================================
+ Hits       165723   165725    +2     
- Misses      14949    14953    +4     
+ Partials     6765     6759    -6     

see 24 files with indirect coverage changes

@@ -159,6 +160,31 @@ const posixTestCases = [
{ path: '/🚀', expected: 'file:///%F0%9F%9A%80' },
// "unsafe" chars
{ path: '/foo\r\n\t<>"#%{}|^[\\~]`?bar', expected: 'file:///foo%0D%0A%09%3C%3E%22%23%25%7B%7D%7C%5E%5B%5C%7E%5D%60%3Fbar' },
// All of the 16-bit UTF-16 chars
{
path: `/${String.fromCharCode(...Array.from({ length: 0x7FFF }, (_, i) => i))}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be gentle with the call stack.

Suggested change
path: `/${String.fromCharCode(...Array.from({ length: 0x7FFF }, (_, i) => i))}`,
path: `/${Array.from({ length: 0x7FFF }, (_, i) => String.fromCharCode(i)).join('')}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Spreading 0x7fff elements would work on current V8's defaults, but it exhausts about 30% of hard limit. I'd say, it's an example of borderline safe code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no reason for V8 to drastically reduce that max stack size value – and if it does, the test would fail, which seems like something we'd like to catch. Anyway, I'd rather keep it as is though I don't feel strongly, so if you do I'll take your suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

It's non-blocking (mainly because we already have a few tests with even bigger stack usage... which IMHO should be fixed), but I feel strongly that we should keep test code safe and not failing under non-default environments (e.g. it's reasonable to lower ulimit -s or V8's --stack-size if we want to spawn a lot of threads and minimize memory overhead), at least where it's easy to fix.
If we want to catch a potential regression in effective stack size, a separate explicit test for "minimal/recommended/default" amounts of what can be spreaded/.apply()'d/etc. would make more sense than seemingly-unrelated failures from random tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it hard to believe that calling String.fromCharCode 32k times with 1 argument would be more memory efficient than calling it once with 32k arguments 🤔 is there a way to validate this hypothesis / observe the effects you're talking about?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not more efficient but it's safer since it doesn't allocate O(n) space in stack at once.

let a, b, c, d, e;

try { // current code
  a = `/${String.fromCharCode(...Array.from({ length: 0x7FFF }, (_, i) => i))}`;
} catch(e) { console.error('a failed:', e); }
try { // suggested code
  b = `/${Array.from({ length: 0x7FFF }, (_, i) => String.fromCharCode(i)).join('')}`;
} catch(e) { console.error('b failed:', e); }
try { // faster code
  c = `/${String.fromCharCode(...Array(0x7FFF).keys())}`;
} catch(e) { console.error('c failed:', e); }
try { // .apply()
  d = `/${String.fromCharCode.apply(String, Array.from({ length: 0x7FFF }, (_, i) => i))}`;
} catch(e) { console.error('d failed:', e); }
try { // smaller chunks
  e = '/';
  for (let i = 0; i < 0x7FFF; i += 1024)
    e += String.fromCharCode(...Array.from({ length: Math.min(1024, 0x7FFF - i) }, (_, $) => i + $));
} catch(e) { console.error('e failed:', e); }

console.log(`b (suggested) size: ${b.length}`);
console.log(`string same as a (current): ${b === a}`);
console.log(`string same as c (faster): ${b === c}`);
console.log(`string same as d (apply): ${b === d}`);
console.log(`string same as e (chunks): ${b === e}`);

node --stack-size=984 test.js would pass.
node --stack-size=256 test.js would fail with RangeError (in case of hitting OS's hard limit, e.g. ulimit -s 256 && node test.js, it would just segfault) when full spread or .apply() are used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants