Skip to content

feat: improve test sandbox#5016

Merged
raymondfeng merged 2 commits intomasterfrom
improve-test-sandbox
Apr 6, 2020
Merged

feat: improve test sandbox#5016
raymondfeng merged 2 commits intomasterfrom
improve-test-sandbox

Conversation

@raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Apr 2, 2020 •
edited
Loading

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 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 mentioned this pull request Apr 2, 2020
Closed
14 tasks
bajtos
bajtos suggested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

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'));

@bajtos bajtos added the Testlab @loopback/testlab label Apr 2, 2020
bajtos
bajtos reviewed Apr 2, 2020
View reviewed changes
@raymondfeng raymondfeng force-pushed the improve-test-sandbox branch from d36a809 to 2fd7113 Compare April 2, 2020 16:48
@raymondfeng
Copy link
Contributor Author

raymondfeng commented Apr 2, 2020

As usual, the PR would be easier to review if the testlab change was made in a standalone PR.

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 raymondfeng requested a review from bajtos April 2, 2020 16:50
@raymondfeng raymondfeng force-pushed the improve-test-sandbox branch from 2fd7113 to c9614d3 Compare April 2, 2020 16:56
@dhmlau dhmlau added this to the April 2020 milestone Apr 2, 2020
@raymondfeng raymondfeng force-pushed the improve-test-sandbox branch 3 times, most recently from 37ea12f to 00047ac Compare April 3, 2020 01:00
@bajtos
Copy link
Member

bajtos commented Apr 3, 2020

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.

👍

I'll defer the final review until #5014 is landed and this PR rebased.

bajtos
bajtos reviewed Apr 3, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Few more comments to consider.

@raymondfeng raymondfeng force-pushed the improve-test-sandbox branch 3 times, most recently from 5f7412d to 21c8fba Compare April 3, 2020 17:00
@raymondfeng raymondfeng requested a review from bajtos April 3, 2020 18:06
bajtos
bajtos approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

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?

examples/file-transfer/src/__tests__/acceptance/test-helper.ts Outdated

export function getSandbox() {
const sandbox = new TestSandbox(path.resolve(__dirname, '../../../.sandbox'));
// dist/.sandbox
Copy link
Member

@bajtos bajtos Apr 6, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@raymondfeng raymondfeng Apr 6, 2020

Choose a reason for hiding this comment

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

Sorry for the confusion. We do use a unique subdir now. I fixed the comment.

packages/testlab/README.md

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.
Copy link
Member

@bajtos bajtos Apr 6, 2020

Choose a reason for hiding this comment

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

+1 for adding docs for TestSandbox 👏

@raymondfeng
feat(testlab): introduce TestSandboxOptions to control sanbox creation
c7645db 
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'});
```
@raymondfeng
feat(example-file-transfer): make storage directory configurable via …
d1b9137 
…binding

This is required when the same application is run with different sandbox
directory as the storage for file upload/download.
@raymondfeng raymondfeng force-pushed the improve-test-sandbox branch from 21c8fba to d1b9137 Compare April 6, 2020 16:06
@raymondfeng raymondfeng merged commit df82f54 into master Apr 6, 2020
@raymondfeng raymondfeng deleted the improve-test-sandbox branch April 6, 2020 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agnes512 agnes512 Awaiting requested review from agnes512

@deepakrkris deepakrkris Awaiting requested review from deepakrkris

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

@jannyHou jannyHou Awaiting requested review from jannyHou

@nabdelgadir nabdelgadir Awaiting requested review from nabdelgadir

2 more reviewers

@bajtos bajtos bajtos approved these changes

@emonddr emonddr emonddr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

breaking-change Testlab @loopback/testlab

Projects

None yet

Milestone

April 2020

Development

Successfully merging this pull request may close these issues.

4 participants

@raymondfeng @bajtos @emonddr @dhmlau