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

chore(e2e-tests): add TestShell#eventually and refactor TestShell#executeLine and TestShell#waitForPrompt #2171

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kraenhansen
Copy link
Contributor

This adds a TestShell#eventually which is designed to be a drop-in replacement for the timing based eventually utility.

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 the TestShell receives 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's executeLine to await a prompt before writing the input instead of simply awaiting the prompt afterwards. This is to fix a race which occurs if shell.writeInputLine is called directly from a test and an eventually polls for the output to contain something. In which case the prompt won't be awaited and an executeLine will incorrectly pick up the prompt from the previous command:

it('throws if pwd is wrong', async function () {
await shell.executeLine(`use ${dbName}`);
shell.writeInputLine('db.auth("anna", "pwd2")');
await eventually(
() => {
shell.assertContainsError('Authentication failed');
},
{ timeout: 40000 }
);
expect(
await shell.executeLine('db.runCommand({connectionStatus: 1})')
).to.include('authenticatedUsers: []');

This also refactors TestShell's waitForPrompt to use the new shell.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 in executeLine.

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 eventually in the "e2e Analytics Node" before hook, as it was often failing locally and the timeout of the hook itself was already 60s.

@kraenhansen kraenhansen self-assigned this 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>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

PROMPT_PATTERN.test(lastLine),
`Expected a prompt (last line was "${lastLine}")`
);
return lastLine;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now returning the prompt since we need this in executeLine to omit it from the output.

@kraenhansen kraenhansen force-pushed the kh/e2e-test-shell-refactored branch 2 times, most recently from c4b9f35 to 0601808 Compare September 26, 2024 09:36
Base automatically changed from kh/e2e-test-shell-refactored to main October 1, 2024 18:11
@kraenhansen
Copy link
Contributor Author

It seems this is agitating a few of the flaky tests a bit more. Specifically, I'm seeing these failures quite frequently:

  • FLE tests 7.0+ allows explicit enryption with bypassQueryAnalysis: AssertionError: expected '... ... ... ... \n' to include 'ghjk'
  • e2e config, logging and rc file in fully accessible environment update notification shows an update notification if a newer version is available: Error: Timed out (waited 10000ms): ENOENT: no such file or directory, open '/data/mci/907dee6684bf664703662d882f050b2c/src/tmp/test/cli-repl-home-1727870787014-0.3484944880719283/.mongodb/REDACTED:notary_signing_key_name/update-metadata.json'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant