-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Refactor test-readline-async-iterators into a benchmark #49237
Refactor test-readline-async-iterators into a benchmark #49237
Conversation
Running the benchmark on my system produces following results:
|
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.
lgtm
I see two tests failed. For commit message should I change commit and push or it is ok and will be changed while merging? |
Hi @mcollina, sorry you may be busy, just wanted to know if there are any changes needed from my side on this. |
Hello, your first commit message has an incorrect prefix; it can be formatted as follows: test: refactor test-readline-async-iterators into a benchmark |
6a7950d
to
429e78d
Compare
@mertcanaltin thanks, I have updated |
I am not sure why 3 of the tests are stuck in InProgress state. Can someone help? |
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.
lgtm
Logs from |
@nodejs/build @nodejs/testing could somebody take a look at this? It keeps failing but there are less tests, not more tests, so they should not be. |
I started a new CI -- the only failure was a temporary infra issue while we've been adding gcc 10 to the test machines (this particular failure fixed in nodejs/build#3495). One resume CI later and the Jenkins CI has passed for this PR. |
Commit Queue failed- Loading data for nodejs/node/pull/49237 ✔ Done loading data for nodejs/node/pull/49237 ----------------------------------- PR info ------------------------------------ Title Refactor test-readline-async-iterators into a benchmark (#49237) Author Shubham Pandey (@shubham9411) Branch shubham9411:shubham9411/readline-iterable-benchmark -> nodejs:main Labels readline, test, benchmark, author ready, commit-queue-squash Commits 2 - test: refactor test-readline-async-iterators into a benchmark - empty commit Committers 1 - Shubham Pandey PR-URL: https://github.com/nodejs/node/pull/49237 Fixes: https://github.com/nodejs/node/issues/49224 Reviewed-By: Matteo Collina ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/49237 Fixes: https://github.com/nodejs/node/issues/49224 Reviewed-By: Matteo Collina -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - test: refactor test-readline-async-iterators into a benchmark ⚠ - empty commit ℹ This PR was created on Sat, 19 Aug 2023 05:18:29 GMT ✔ Approvals: 1 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49237#pullrequestreview-1609041171 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-09-21T17:12:52Z: https://ci.nodejs.org/job/node-test-pull-request/54135/ ℹ Last Benchmark CI on 2023-09-07T09:20:49Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1365/ - Querying data for job/node-test-pull-request/54135/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6274851210 |
Hey @mcollina, I pushed one empty commit just to trigger the CI pipeline. Do I have to remove it as commit queue is failing. Thanks |
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.
lgtm
No need |
Landed in c0b4208 |
PR-URL: nodejs#49237 Fixes: nodejs#49224 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Refactor test-readline-async-iterators into a benchmark
Fixes: #49224
Added old way of readline async iterator to benchmark.