fix(cli) Add back the test for cloneExampleFromGitHub #1011
Conversation
shimks
left a comment
shimks
left a comment
There was a problem hiding this comment.
LGTM. Just one thing that was missed I think.
packages/cli/.gitignore
Outdated
| @@ -0,0 +1 @@ | |||
| .sandbox | |||
There was a problem hiding this comment.
We can bring this up to the root-level IIRC
There was a problem hiding this comment.
In hindsight, I should've made the commit look like this rather than disabling the test.
I'd agree with @shimks ' review, just that one minor fixup but otherwise it's good to go.
EDIT: Your commit message is missing the colon between the scope and change statement, and the change statement must start with a lowercase letter.
fix(cli): add back the test for cloneExampleFromGithub
kjdelisle
force-pushed
the
fix/clone-example-test
branch
from
0c481f6 to
cbda602
Compare
|
I've made an amendment to the commit to fixup the message. |
Relax the assertions performed by the test to allow changes reorganizing files around. Rework the test to async/await style, now that we no longer support Node.js 6.x Change the sandbox directory from "test/sandbox" to ".sandbox" to prevent interference with code scanning "test" for test files to run.
kjdelisle
force-pushed
the
fix/clone-example-test
branch
from
cbda602 to
35a97a6
Compare
|
I've also applied the feedback and hoisted the .sandbox into the top-level ignore. Normally, I'd leave this alone, but I do want to get that test coverage back online ASAP (I hate disabling tests for any but the very best of reasons). |
virkt25
left a comment
virkt25
left a comment
There was a problem hiding this comment.
LGTM. Two comments, not blockers but would help with consistency imo.
| let sandbox; | ||
|
|
||
| describe.skip('cloneExampleFromGitHub (SLOW)', function() { | ||
| describe('cloneExampleFromGitHub (SLOW)', function() { |
There was a problem hiding this comment.
You can remove the word function and use describe('cloneExampleFromGitHub (SLOW)', () => { to be consistent with the code base.
There was a problem hiding this comment.
We cannot - the next line is calling this.timeout(). This is one of the gotchas of using arrow functions with Mocha.
| 'src/index.ts', | ||
| ]); | ||
|
|
||
| const packageJson = JSON.parse(fs.readFileSync(`${outDir}/package.json`)); |
There was a problem hiding this comment.
I think it might make sense to promisify readFile and use that with await to be consistent?
I personally prefer to keep package-specific |
Relax the assertions performed by the test to allow changes reorganizing files around.
Rework the test to async/await style, now that we no longer support Node.js 6.x
Change the sandbox directory from "test/sandbox" to ".sandbox" to prevent interference with code scanning "test" for test files to run.
This is a follow-up for #1003 which disabled this problematic test.
Checklist
npm testpasses on your machinepackages/cliwere updatedpackages/example-*were updated