chore(e2e-tests): add TestShell#eventually and refactor TestShell#executeLine and TestShell#waitForPrompt#2171
Draft
kraenhansen wants to merge 7 commits intomainfrom
Draft
chore(e2e-tests): add TestShell#eventually and refactor TestShell#executeLine and TestShell#waitForPrompt#2171kraenhansen wants to merge 7 commits intomainfrom
TestShell#eventually and refactor TestShell#executeLine and TestShell#waitForPrompt#2171kraenhansen wants to merge 7 commits intomainfrom
Conversation
kraenhansen
commented
Sep 18, 2024
| * Like the `eventually` utility, but instead of calling the callback on a timer, | ||
| * the callback is called as output is emitted. | ||
| */ | ||
| eventually<T = unknown>( |
Contributor
Author
There was a problem hiding this comment.
I might need help to pick a better name for it 🤔 The reason I choose this, is because it acts as a replacement for the pull-based eventually utility used elsewhere.
kraenhansen
commented
Sep 18, 2024
| PROMPT_PATTERN.test(lastLine), | ||
| `Expected a prompt (last line was "${lastLine}")` | ||
| ); | ||
| return lastLine; |
Contributor
Author
There was a problem hiding this comment.
Now returning the prompt since we need this in executeLine to omit it from the output.
c4b9f35 to
0601808
Compare
6e2024a to
49c8443
Compare
Contributor
Author
|
It seems this is agitating a few of the flaky tests a bit more. Specifically, I'm seeing these failures quite frequently:
|
544b16d to
25ac37c
Compare
added 7 commits
October 8, 2024 20:05
Remove unused imports
In an attempt to fix flakiness of "fails fast for ENOTFOUND/EINVAL errors" in e2e.spec.ts
a6f6834 to
8179ade
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds a
TestShell#eventuallywhich is designed to be a drop-in replacement for the timing basedeventuallyutility.Tests can now call
await shell.eventually(() => { /* block which conditionally throws */ })and pass a callback which will get called whenever the child process spawned by theTestShellreceives output. This effectively acts as a push-stream to allow for faster execution of tests (5min 4sec before vs 4min 20sec after) and less noise if logging in these callbacks while developing tests.This refactors
TestShell'sexecuteLineto await a prompt before writing the input instead of simply awaiting the prompt afterwards. This is to fix a race which occurs ifshell.writeInputLineis called directly from a test and aneventuallypolls for the output to contain something. In which case the prompt won't be awaited and anexecuteLinewill incorrectly pick up the prompt from the previous command:mongosh/packages/e2e-tests/test/e2e-auth.spec.ts
Lines 738 to 749 in a520eb1
This also refactors
TestShell'swaitForPromptto use the newshell.eventually(push streaming) and only consider the last line of the output determining if the output has a prompt. I believe this is now possible because of a fix for the race inexecuteLine.Note: This is based on the changes introduced in #2170 and needs a rebase once that is merged.
Note: This also sneaks in a bump of the timeout passed to
eventuallyin the "e2e Analytics Node" before hook, as it was often failing locally and the timeout of the hook itself was already 60s.