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: simplify test-gc-{http-client,net}-* #42782

Closed
wants to merge 1 commit into from

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Apr 19, 2022

Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

@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 Apr 19, 2022
@lpinca lpinca force-pushed the simplify/test-gc-xxx branch 2 times, most recently from 59d8f04 to 5324b0b Compare April 19, 2022 13:48
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2022
@nodejs-github-bot

This comment was marked as outdated.

@lpinca lpinca changed the title test: simplify test-gc-* test: simplify test-gc-{http-client,net}* Apr 19, 2022
@lpinca lpinca changed the title test: simplify test-gc-{http-client,net}* test: simplify test-gc-{http-client,net}-* Apr 19, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2022
@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 21, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Comment on lines 60 to 62
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
return;
}
}
setImmediate(status);

Copy link
Member Author

@lpinca lpinca Apr 28, 2022

Choose a reason for hiding this comment

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

This is only a cosmetic change that is inconsistent with the original refactor done in 47ecf2060343. I would prefer to not apply it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, feel free to ignore and land as is, LGTM anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll apply it, but it's a little annoying to rerun CI for something like this.

Comment on lines 68 to 69
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
return;
}
}
setImmediate(status);

Comment on lines 71 to 73
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
} else {
setImmediate(status);
}
} else {
setImmediate(status);
}
return;
}
}
setImmediate(status);

Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: nodejs@47ecf2060343
Refs: nodejs@7ce8403ef1a6
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 28, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 29, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/42782
✔  Done loading data for nodejs/node/pull/42782
----------------------------------- PR info ------------------------------------
Title      test: simplify test-gc-{http-client,net}-* (#42782)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     lpinca:simplify/test-gc-xxx -> nodejs:master
Labels     test, author ready, needs-ci
Commits    1
 - test: simplify test-gc-{http-client,net}-*
Committers 1
 - Luigi Pinca 
PR-URL: https://github.com/nodejs/node/pull/42782
Refs: https://github.com/nodejs/node/commit/47ecf2060343
Refs: https://github.com/nodejs/node/commit/7ce8403ef1a6
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42782
Refs: https://github.com/nodejs/node/commit/47ecf2060343
Refs: https://github.com/nodejs/node/commit/7ce8403ef1a6
Reviewed-By: James M Snell 
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - test: simplify test-gc-{http-client,net}-*
   ℹ  This PR was created on Tue, 19 Apr 2022 13:41:44 GMT
   ✔  Approvals: 2
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/42782#pullrequestreview-947803586
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42782#pullrequestreview-956901494
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-04-28T22:34:39Z: https://ci.nodejs.org/job/node-test-pull-request/43757/
- Querying data for job/node-test-pull-request/43757/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/2247249270

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Apr 29, 2022
lpinca added a commit that referenced this pull request Apr 29, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@lpinca
Copy link
Member Author

lpinca commented Apr 29, 2022

Landed in 02e0c17.

@lpinca lpinca closed this Apr 29, 2022
targos pushed a commit that referenced this pull request May 2, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@targos targos mentioned this pull request May 2, 2022
@lpinca lpinca deleted the simplify/test-gc-xxx branch May 10, 2022 08:17
juanarbol pushed a commit that referenced this pull request May 31, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: 47ecf2060343
Refs: 7ce8403ef1a6

PR-URL: #42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Instead of sending/creating a fixed number of requests/connections,
detect when GC has started and stop sending requests/creating
connections at that point.

Refs: nodejs/node@47ecf2060343
Refs: nodejs/node@7ce8403ef1a6

PR-URL: nodejs/node#42782
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. 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