-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: use sandbox.path instead of SANDBOX to access the sandbox directory #5014
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-posting from #5016 (review):
I love the idea of changing the code to use sandbox.path
. 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'));
324d86a
to
9ea5243
Compare
@bajtos The SANDBOX constants are removed. |
6d9ff7a
to
d67658a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for removing SANDBOX constants 👏 The pull request is almost ready now 🏁
…irectory The starting path for a sandbox is not necessarily the target directory as we may create a unique subdir under the path to ensure sandboxes can be used in parallel testing.
d67658a
to
7e2ade7
Compare
Step 1 for #5011
Refactor test sandbox usage to use
sandbox.path
to access the sandbox directory as we'll create unique subdirs for different sandbox instances.Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈