-
Notifications
You must be signed in to change notification settings - Fork 29k
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: add coverage for webstorage quota #53964
test: add coverage for webstorage quota #53964
Conversation
7b54022
to
06067e3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Edit: hmm actually - why it needs a rebase if it does not have merge conflict (it only adds a new test) 🤔 probably rebase failure was for something else, will pay more attention to the cause when I see it next time. After the rebase it did run ok though. |
06067e3
to
c44e668
Compare
Do you mind taking a look @cjihrig 🙏 |
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.
This looks fine to me. I think it could be improved in a few ways:
- Remove the loops. Unless I'm missing something, it seems like extra work that makes the test harder to reason about.
- Perform the entire localStorage test in a single child process like the sessionStorage test.
- Since you're matching stderr, add something to the output so you can verify that the assertion came from the place you intended. In the sessionStorage test I don't think we can differentiate a failure in the loop vs the
sessionStorage.anything
line.
I think something like this would work:
const max = 10 * 1024 * 1024;
sessionStorage['i'.repeat(max / 2)] = '';
console.error('filled');
sessionStorage['x'] = '';
Thanks for the review 🙏
Done, I remembered I hit a limit error from V8 about string size being too big but I cannot reproduce it anymore.
Done
Good suggestion, done ✔️ |
Commit Queue failed- Loading data for nodejs/node/pull/53964 ✔ Done loading data for nodejs/node/pull/53964 ----------------------------------- PR info ------------------------------------ Title test: add coverage for webstorage quota (#53964) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch jakecastelli:ref-53871-max-storage-quota -> nodejs:main Labels test, author ready, needs-ci, commit-queue-squash Commits 2 - test: add coverage for webstorage quota - fixup! improve test Committers 1 - jakecastelli <jake.yuesong@gmail.com> PR-URL: https://github.com/nodejs/node/pull/53964 Refs: https://github.com/nodejs/node/issues/53871 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53964 Refs: https://github.com/nodejs/node/issues/53871 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 20 Jul 2024 04:20:43 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53964#pullrequestreview-2190383507 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/53964#pullrequestreview-2208922439 ✔ - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/53964#pullrequestreview-2205194172 ✘ GitHub CI is still running ℹ Last Full PR CI on 2024-07-29T10:32:41Z: https://ci.nodejs.org/job/node-test-pull-request/60719/ - Querying data for job/node-test-pull-request/60719/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10207447292 |
Commit Queue failed- Loading data for nodejs/node/pull/53964 ✔ Done loading data for nodejs/node/pull/53964 ----------------------------------- PR info ------------------------------------ Title test: add coverage for webstorage quota (#53964) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch jakecastelli:ref-53871-max-storage-quota -> nodejs:main Labels test, author ready, needs-ci, commit-queue-squash Commits 2 - test: add coverage for webstorage quota - fixup! improve test Committers 1 - jakecastelli <jake.yuesong@gmail.com> PR-URL: https://github.com/nodejs/node/pull/53964 Refs: https://github.com/nodejs/node/issues/53871 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53964 Refs: https://github.com/nodejs/node/issues/53871 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> -------------------------------------------------------------------------------- ℹ This PR was created on Sat, 20 Jul 2024 04:20:43 GMT ✔ Approvals: 3 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53964#pullrequestreview-2190383507 ✔ - Colin Ihrig (@cjihrig): https://github.com/nodejs/node/pull/53964#pullrequestreview-2208922439 ✔ - Minwoo Jung (@JungMinu): https://github.com/nodejs/node/pull/53964#pullrequestreview-2205194172 ✘ GitHub CI is still running ℹ Last Full PR CI on 2024-08-01T23:49:52Z: https://ci.nodejs.org/job/node-test-pull-request/60719/ - Querying data for job/node-test-pull-request/60719/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/10214193300 |
Landed in 0c1877a |
Refs: #53871