fix(boot): make sure sandbox directories are removed after tests

[raymondfeng]

Extracted from #5011. It cleans up the tests to avoid conflicting sandbox paths for parallel testing.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[bajtos]

-1

We have been intentionally cleaning sandbox directory before the tests start and keep the files created by tests around. That way, when a test fails, it's easy to inspect what happened in the test by looking at the files inside sandbox. Quoting from our best practices at https://loopback.io/doc/en/lb4/Testing-your-application.html#clean-the-database-before-each-test:

Start with a clean database before each test. This may seem counter-intuitive: why not reset the database after the test has finished? When a test fails and the database is cleaned after the test has finished, then it’s difficult to observe what was stored in the database and why the test failed. When the database is cleaned in the beginning, then any failing test will leave the database in the state that caused the test to fail.

With 5720 landed, how do you @raymondfeng envision to debug failed CLI tests? Are we going to have to run the tests from the debugger and setup a breakpoint before the files are deleted, so that we can inspect them? I find that rather annoying.

[hacksparrow]

We have been intentionally cleaning sandbox directory before the tests start and keep the files created by tests around.

Yes, that's the way to do it. @raymondfeng let's revert the change. As long as the sandbox directories are in .gitignore, they should not matter.

[raymondfeng]

I have a different view here. IMO, tests should clean up themselves so that no side effects are left on the system, such as files, env vars, or network ports.

We also have to acknowledge that reset in beforeEach has similar consequences as Mocha will try to run all tests even when one of them fails. Let's say we have multiple tests in one file that share the same beforeEach(() => sandbox.reset()), how would you depend on the left-over sandbox files if test1 fails and test2 succeeds?

Sure, it would be easier to leave the sandbox files on the disk to troubleshoot these particular test failures. But we can also argue if we should also leave the server open without calling app.stop.

IMO, troubleshooting test failures is not different from debugging code issues. I'm not saying that we have to use debugger all the time. Please note that mocha allows you to skip hooks with this.skip(). We use similar things such as it.only or describe.only to isolate test issues.

[bajtos]

I have a different view here. IMO, tests should clean up themselves so that no side effects are left on the system, such as files, env vars, or network ports.

Sure, I agree that in general, tests should not leave up mess on the system. However, in this particular case, I feel the developer experience is important than theoretical purity.

What practical difficulties have you encountered with the original solution where we keep sandbox files around? I believe the problem with parallel mocha has been solved by making sandbox directories unique. What else is there to solve?

We also have to acknowledge that reset in beforeEach has similar consequences as Mocha will try to run all tests even when one of them fails. Let's say we have multiple tests in one file that share the same beforeEach(() => sandbox.reset()), how would you depend on the left-over sandbox files if test1 fails and test2 succeeds?

Good point. I believe this has been solved by your recent PR where you modified sandbox to use unique directory names. Am I mistaken?

---

IMO, this pull request should not have been landed because we have clearly not reached consensus yet.
If I am reading git metadata correctly, then the PR was opened around 9.40pm my time and landed around 11.40pm my time, less than two hours after being opened for review. That certainly didn't help to get everybody to agree with the proposal.

I opened #5738 to revert the changes in this pull request so that we can discuss the problems you are trying to solve and find a better solution, one that we will all agree with.

Few ideas to consider:

• Remove all sandbox directories from an npm script, before we execute mocha. This will ensure old sandbox directories from previous test runs are not kept around.
• Implement an after/afterEach hook that will delete sandbox directory for passed tests only, but keep it around for tests that failed. I think Mocha provides success/error flag in hook context.

[bajtos]

Two more comments:

• We are using sandbox directories in more packages than just @loopback/boot, it would be great to have the same behavior everywhere.
• Another option for keeping decent DX is to introduce a new ENV variable (e.g. KEEP_SANDBOX=1) to temporarily disable the cleanup behavior.

[raymondfeng]

IMO, this pull request should not have been landed because we have clearly not reached consensus yet.
If I am reading git metadata correctly, then the PR was opened around 9.40pm my time and landed around 11.40pm my time, less than two hours after being opened for review. That certainly didn't help to get everybody to agree with the proposal.

I think we need to establish a protocol here. The pull requests are taking too long to land and it's not practical to get everybody to review each PR. IMO, it has always possible and encouraged to add more feedbacks even after a PR is landed. We can then decide if we should have follow-up PRs to address such comments or revert the original PR if it's severely flawed. Reverting a PR should be the last resort. It's natural to arrive an elegant solution over iterations. BTW, reverting a PR makes it difficult to fix/improve the original PR.

[raymondfeng]

What practical difficulties have you encountered with the original solution where we keep sandbox files around? I believe the problem with parallel mocha has been solved by making sandbox directories unique. What else is there to solve?

When I was trying to troubleshoot test failures, I found there were many left-over subdirectories under .sandbox. This prevented me from knowing which directory I should look into.

Good point. I believe this has been solved by your recent PR where you modified sandbox to use unique directory names. Am I mistaken?

Not really. We reuse the sandbox path for all tests inside the same file. Let's assume test1 generates some files in the sandbox but it fails. Mocha continues to try test2 and the beforeEach hook resets the sandbox. As a result, we lost the sandbox for test1.

We are using sandbox directories in more packages than just @loopback/boot, it would be great to have the same behavior everywhere.

+1 to be consistent.

Another option for keeping decent DX is to introduce a new ENV variable (e.g. KEEP_SANDBOX=1) to temporarily disable the cleanup behavior.

Do we want to honor this env var in @loopback/testlab for both reset and delete?

[raymondfeng]

@bajtos Let's make the discussion more concrete with the following example test file (which is typical in our test suite):

1. const sandbox = new TestSandBox(...);
2. beforeEach => sandbox.reset()
3. test1
4. test2

At step 1, .sandbox/<unique-subdir> is created.
Before step 3, the sandbox is reset
test1 fails
Before step 4, the sandbox is reset
test2 succeeds

Now you have the sandbox files created by test2 left on the file system. They won't help you troubleshoot test1 at all.

My introduction of after hook to delete the sandbox does not make things worse. Please also consider the following fact:

When you run the tests multiple times without delete, you'll see many subdirs under .sandbox over time. Whenever a test fails, you have no clue where to find the files for the failing test.

[bajtos]

Now you have the sandbox files created by test2 left on the file system. They won't help you troubleshoot test1 at all.

That's true.

I often re-run the failing test in isolation via lb-mocha -g "test2". After that, I get the problematic files in the sandbox ready for inspection.

There is one thing I find confusing here - I thought sandbox.reset is going to create a unique subdirectory, in which case we would get one dir for test1 and another for test2. Are you saying that we create the unique subdir per sandbox instance (i.e. typically per test file)?

When you run the tests multiple times without delete, you'll see many subdirs under .sandbox over time. Whenever a test fails, you have no clue where to find the files for the failing test.

I agree with you this is a problem. I am thinking about two ways how to improve the situation, they are not mutually exclusive:

• Wipe the entire top-level sanbox directory before running the tests. I was initially thinking about adding rimraf test/sandbox to npm test script, but perhaps we can leverage recently introduced Mocha root/global hooks for that? The non-trivial part is how to allow individual packages to define these hooks in such way that the monorepo-level test runner can pick them up.
• Modify the cleanup routine to keep the sandbox around only when the test failed and print a helpful message to make the sandbox directory easier to find.

Thoughts?

@hacksparrow you were in favor of reverting this PR, do you have any suggestions how to address the practical problems described by @raymondfeng while keeping troubleshooting easy?

[raymondfeng]

There is one thing I find confusing here - I thought sandbox.reset is going to create a unique subdirectory, in which case we would get one dir for test1 and another for test2. Are you saying that we create the unique subdir per sandbox instance (i.e. typically per test file)?

sandbox.reset() keeps the same subdir and removes all files in the directory. We create a unique subdir per sandbox instance. This matches my expectations as reset restores the initial state of the sandbox instance - an empty dir.