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: add coverage for webstorage quota #53964

Closed
wants to merge 2 commits into from
Closed

test: add coverage for webstorage quota #53964

wants to merge 2 commits into from

Conversation

jakecastelli
Copy link
Member

Refs: #53871

@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 Jul 20, 2024
@jakecastelli jakecastelli 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 Jul 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 25, 2024
@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
@jakecastelli
Copy link
Member Author

jakecastelli commented Jul 27, 2024 •
edited
Loading

CI failed due to need a rebase

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.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@jakecastelli
Copy link
Member Author

Do you mind taking a look @cjihrig 🙏

cjihrig
cjihrig approved these changes Jul 28, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a 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:

  1. Remove the loops. Unless I'm missing something, it seems like extra work that makes the test harder to reason about.
  2. Perform the entire localStorage test in a single child process like the sessionStorage test.
  3. 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'] = '';

@jakecastelli
Copy link
Member Author

Thanks for the review 🙏

Remove the loops. Unless I'm missing something, it seems like extra work that makes the test harder to reason about.

Done, I remembered I hit a limit error from V8 about string size being too big but I cannot reproduce it anymore.

Perform the entire localStorage test in a single child process like the sessionStorage test.

Done

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.

Good suggestion, done ✔️

@jakecastelli jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 29, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@jakecastelli jakecastelli added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 30, 2024
@jakecastelli jakecastelli added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 1, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 1, 2024
@nodejs-github-bot
Copy link
Collaborator

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 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53964
Refs: https://github.com/nodejs/node/issues/53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53964
Refs: https://github.com/nodejs/node/issues/53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
--------------------------------------------------------------------------------
   ℹ  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/.ncu
https://github.com/nodejs/node/actions/runs/10207447292

@jakecastelli jakecastelli added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 2, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 2, 2024
@nodejs-github-bot
Copy link
Collaborator

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 <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/53964
Refs: https://github.com/nodejs/node/issues/53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53964
Refs: https://github.com/nodejs/node/issues/53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
--------------------------------------------------------------------------------
   ℹ  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/.ncu
https://github.com/nodejs/node/actions/runs/10214193300

jakecastelli added a commit that referenced this pull request Aug 2, 2024
@jakecastelli
test: add coverage for webstorage quota
0c1877a 
PR-URL: #53964
Refs: #53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@jakecastelli jakecastelli removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Aug 2, 2024
@jakecastelli
Copy link
Member Author

Landed in 0c1877a

targos pushed a commit that referenced this pull request Aug 14, 2024
@jakecastelli @targos
test: add coverage for webstorage quota
5dbff81 
PR-URL: #53964
Refs: #53871
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Aug 19, 2024
Merged
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@JungMinu JungMinu JungMinu approved these changes

@cjihrig cjihrig cjihrig 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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jakecastelli @nodejs-github-bot @jasnell @cjihrig @JungMinu @marco-ippolito @targos