feat: improve test sandbox #5016
Conversation
raymondfeng
requested review from
agnes512,
bajtos,
deepakrkris,
emonddr,
hacksparrow,
jannyHou and
nabdelgadir
as code owners
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
I love the idea of changing the code to use sandbox.path and also to improve TestSandbox to create a unique sandbox directory 👏
As usual, the PR would be easier to review if the testlab change was made in a standalone PR.
Considering that SANDBOX_PATH should not be used any more, I think our code would be much less error-prone if the constant was removed completely.
Now:
const SANDBOX_PATH = path.resolve(__dirname, '../.sandbox');
const sandbox = new TestSandbox(SANDBOX_PATH);My proposal:
const sandbox = new TestSandbox(path.resolve(__dirname, '../.sandbox'));
packages/boot/src/__tests__/acceptance/application-metadata.booter.acceptance.ts
Outdated
Show resolved
Hide resolved
raymondfeng
force-pushed
the
improve-test-sandbox
branch
from
d36a809 to
2fd7113
Compare
Please ignore the 1st commit. As I pointed out in the PR, it depends on #5014. After 5014 is landed, I'll rebase to remove the commit. |
raymondfeng
force-pushed
the
improve-test-sandbox
branch
from
2fd7113 to
c9614d3
Compare
raymondfeng
force-pushed
the
improve-test-sandbox
branch
3 times, most recently
from
37ea12f to
00047ac
Compare
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
Few more comments to consider.
packages/boot/src/__tests__/acceptance/application-metadata.booter.acceptance.ts
Outdated
Show resolved
Hide resolved
raymondfeng
force-pushed
the
improve-test-sandbox
branch
3 times, most recently
from
5f7412d to
21c8fba
Compare
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
LGTM, but please consider improving the comment in file-transfer tests.
To be honest, I don't understand why you cannot use a unique dir name in tests, considering that the upload target dir is configurable?
|
|
||
| export function getSandbox() { | ||
| const sandbox = new TestSandbox(path.resolve(__dirname, '../../../.sandbox')); | ||
| // dist/.sandbox |
There was a problem hiding this comment.
Can you please improve the comment and explain why it's ok to use the same directory dist/.sandbox when running the tests in parallel?
There was a problem hiding this comment.
Sorry for the confusion. We do use a unique subdir now. I fixed the comment.
|
|
||
| Many tests need use a temporary directory as the sandbox to mimic a tree of | ||
| files. The `TestSandbox` class provides such facilities to create and manage a | ||
| sandbox on the file system. |
There was a problem hiding this comment.
+1 for adding docs for TestSandbox 👏
BREAKING CHANGE: The TestSandbox constructor changes its signature and behavior
now. It used to take a `path` as the top-level directory of the sandbox. The
new style is illustrated below.
```ts
// Create a sandbox as a unique temporary subdirectory under the rootPath
const sandbox = new TestSandbox(rootPath);
const sandbox = new TestSandbox({subdir: true});
// Create a sandbox in the root path directly
// This is same as the old behavior
const sandbox = new TestSandbox(rootPath, {subdir: false});
const sandbox = new TestSandbox(rootPath, {subdir: '.'});
// Create a sandbox in the `test1` subdirectory of the root path
const sandbox = new TestSandbox(rootPath, {subdir: 'test1'});
```
…binding This is required when the same application is run with different sandbox directory as the storage for file upload/download.
raymondfeng
force-pushed
the
improve-test-sandbox
branch
from
21c8fba to
d1b9137
Compare
Step 3 of #5011
Depends on #5014 (included here as the 1st commit so that CI can pass).
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm testpasses on your machinepackages/cliwere updatedexamples/*were updated👉 Check out how to submit a PR 👈