Skip to content

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jun 15, 2020

This is rework of #5720.

There are 3 commits:

1. Use unique sandbox subdirs for parallel testing (must-have) Extracted to #5747

  1. Add KEEP_TEST_SANDBOX env var to keep files for sandbox.delete (nice-to-have)

  2. Add after hook to delete test sandbox directories (should-have)

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 👈

@raymondfeng raymondfeng changed the title fix: improve sandbox tests fix: clean up sandbox directories after tests Jun 16, 2020
await remove(this.path);
if (!process.env.KEEP_TEST_SANDBOX) {
await remove(this.path);
}
Copy link
Member

Choose a reason for hiding this comment

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

I am little bit reluctant to implement env-based behavior inside delete, I think it could be surprising. Can we implement a new method please and find a descriptive name that will make it more clear what is happening or where it should be used?

Since our motivation is to run this code in after/afterEach hooks, maybe cleanupAfterTests could be a better name?

after('delete sandbox', () => sandbox.cleanupAfterTests());

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.

I would like to investigate a way how to let the sandbox cleanup routine detect when a test failed and print a helpful message (something along the lines "sandbox for test XYZ is in directory ABC"). This is probably out of scope of this PR, but if we introduce a new method like cleanupAfterTests, then it will be easier to enhance the behavior in the future.

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

I think we should apply ENV-specific behavior in delete only. reset is meant to be called before the tests, I think the test would fail when the sanbox was not cleared because of ENV config.

@stale
Copy link

stale bot commented Dec 25, 2020

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Dec 25, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

@stale stale bot closed this Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants