chore(e2e-tests): refactor TestShell to ensure killAll gets called

[kraenhansen]

I was experiencing the e2e tests hanging after exit, both when ran locally and on CI.

This refactor includes:

• A startTestShell method on the Mocha.Context to ensures a hook is setup to kill all open test shells. Instead of calling TestShell.start most hooks and tests can simply call this.startTestShell instead.
• A ensureTestShellAfterHook function exported from test-shell-context.ts, which tests can optionally call to register the "kill hook" to run before another hook they want to register (this is used only once in e2e.spec.ts to ensure a directory deletion handles after no shells are accessing it).
• A TestShell.assertNoOpenShells static method which will throw if there are any open test shells.
• Deletion of the existing TestShell.runAndGetOutputWithoutErrors method and introduces a waitForCleanOutput method on the TestShell instance.
• Deletion of the existing TestShell.cleanup and TestShell.killall static methods as there are no longer needed when the lifetime is all taken care of automagically 🪄

[addaleax]

Needs a rebase because it had some conflicts with #2164 but otherwise looks good!

[addaleax]

Just a note, not something I have a good solution for off the top of my head, but it's now less obvious that the afterEach hook registered by cleanTestShellsAfterEach() has to run before this one – if that breaks, we'll have a flaky test here that's not going to be trivial to debug.

[kraenhansen]

if that breaks, we'll have a flaky test here

I don't think so. If that happens the assert starting the test server will fail and it'll be obvious what the change needs to be 🤞

Or did you call TestShell.start in an after hook?

[addaleax]

Hm – I was thinking more of the fs.rm() here in this block failing because TestShell instances could still be running. I guess the existing try/catch accounts for that to some degree though.

[kraenhansen]

I was thinking more of the fs.rm() here

Oh, I didn't understand that. Thanks for clarifying that.
We could add another check to assert no servers shells are running?

[addaleax]

I think you mean open shells, right? In any case, the added commit LGTM!

[addaleax]

Fwiw I agree with the idea of making test shell lifetime management more explicit (in particular, explicitly cleaning up started shells), but I'm not sure if it's a good idea to move all usage of TestShell.start() to before hooks just from a practical point of view. A lot of our individual tests start shells with specific parameters that only really apply to that specific test, for example, and giving every single one of those tests its own mocha context() is probably going to impact readability and maintainability significantly.

[kraenhansen]

A lot of our individual tests start shells with specific parameters that only really apply to that specific test, for example, and giving every single one of those tests its own mocha context() is probably going to impact readability and maintainability significantly.

I toyed around with it and reached a similar conclusion, regarding the hooks, now I'm contemplating a different API exposed on the Mocha Context which starts the shell and then registers an after hook (or event listener) to kill the shell.

Then one could simply call this.startTestShell() in a beforeEach, before or it and this would remember to kill and await the exit of the shell.

[kraenhansen]

I pushed something significantly different from my original proposal and updated the description of the PR 🙈

[addaleax]

Should there be an TestShell.assertNoOpenShells() call here? :)

[kraenhansen]

You're right, I somehow mixed that up and thought it had to be here:

mongosh/packages/e2e-tests/test/e2e.spec.ts

Line 1386 in 0601808

TestShell.assertNoOpenShells();

I've added it into e2e-editor.spec.ts 👍

[addaleax]

I don't think we need to deprecate this, there shouldn't be anything wrong with using it apart from accepting the responsibility to manually clean the instance up afterwards.

I'd maybe even consider making this "layering" a bit more explicit, and split out the mocha-specific parts into its own file, so that it's clear that TestShell is now a self-contained class that contains the parts that are independent from mocha usage?

[addaleax]

Actually, thinking about it, do we maybe even want to implement Symbol.asyncDispose here and use await using shell = this.startTestShell(...)?

[kraenhansen]

there shouldn't be anything wrong with using it apart from accepting the responsibility to manually clean the instance up afterwards

I see your point and agree there could be future scenarios where one would want to start the shell directly.

However, I was successful in updating all direct calls to TestServer.start to use startTestShell instead (making it a good candidate for the preferred way of starting test shells) and this is the most effective way I know to nudge myself into the startTestShell-way and clearly signal to future me, that:

1. Although you can still call TestShell.start (since it isn't private), it is not the preferred way.
2. You should be careful if you do call TestShell.start yourself, as you take on responsibilities.

Do you have any suggestions for alternatives to signal this intent? I was considering a console.warn, but that easily gets lost in the output and doesn't give signal when the code is being written.

split out the mocha-specific parts into its own file

Makes sense 👍 I've pushed a commit splitting this and updated the PR description.

[addaleax]

Do you have any suggestions for alternatives to signal this intent?

Honestly, I think just a regular doc comment does the trick here perfectly well 🙂

[kraenhansen]

Let's try 👍 I've pushed a commit with good ol' doc comment 🙂

[kraenhansen]

Merging as the failures left seem unrelated to the changes and I believe I've actually addressed those in #2171