-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
test: add tests for REPL custom evals (second attempt) #57850
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
base: main
Are you sure you want to change the base?
test: add tests for REPL custom evals (second attempt) #57850
Conversation
8c394e5
to
9413135
Compare
9413135
to
1e33c67
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57850 +/- ##
==========================================
+ Coverage 90.22% 90.24% +0.01%
==========================================
Files 633 633
Lines 186871 186880 +9
Branches 36685 36687 +2
==========================================
+ Hits 168612 168649 +37
+ Misses 11042 11033 -9
+ Partials 7217 7198 -19
🚀 New features to boost your workflow:
|
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
There are still failures. Here is one in the latest CI run: https://ci.nodejs.org/job/node-test-commit-linux-containered/50117/nodes=ubuntu2204_sharedlibs_withoutssl_x64/testReport/(root)/parallel/test_repl_custom_eval/
|
oof... 😓 sorry about that, thanks @lpinca, I'll have a look 🙏 |
1e33c67
to
584b6d9
Compare
@lpinca I couldn't really reproduce the issue locally but I refactored the code to make it more robust regarding timing, I hope that this will fix the CI check, could you re-run the tests? 🙏 |
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - test: add tests for REPL custom evals ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/14532544242 |
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - test: add tests for REPL custom evals ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/14833744376 |
584b6d9
to
8196294
Compare
8196294
to
c93bded
Compare
this commit reintroduces the REPL custom eval tests that have been introduced in nodejs#57691 but reverted in nodejs#57793 the tests turned out problematic before because `getReplOutput`, the function used to return the repl output wasn't taking into account that input processing and output emitting are asynchronous operation can can resolve with a small delay
c93bded
to
a98d47f
Compare
Failed to start CI⚠ Commits were pushed since the last approving review: ⚠ - test: add tests for REPL custom evals ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/15100627673 |
this PR reintroduces the REPL custom eval tests that have been introduced in #57691 but reverted in #57793
the tests were working fine locally but caused failures in CI (like this one) the problem, I believe, being that the
getReplOutput
would return theoutput
value right away even though the repl processing is asynchronous. Some small delay there is, I imagine, present in the CI runs and that's what I think was causing the issue.I addressed this problem by returning a promise that resolves to the repl's output when the repl output stream is idle, instead of returning the output right away.
For convenience here's the diff between what I am introducing in this PR vs my previous version: github diff