-
-
Notifications
You must be signed in to change notification settings - Fork 591
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
tests: add e2e tests for plugin #1478
Conversation
Ahh, looking at tests |
@anshumanv strange, CI green |
@evilebottnawi I removed checks for yarn.lock(from both init and loader as either of them fails if checked), fails otherwise 😭 |
@anshumanv problems only locally? |
@evilebottnawi only in MacOS CI, passes locally on MacOS |
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.
Instead of yarn.lock
, you can check for existence of node_modules
folder. yarn.lock
is there for checking whether packages are installed or not by the generator in the end. I think it would help for now.
Let me see if that works. 👍 |
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @rishabh3112 Please review the new changes. |
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.
/cc @webpack/cli-team
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.
Looks good to me. Left a few suggestions.
test/init/auto/init-auto.test.js
Outdated
@@ -33,7 +33,7 @@ describe('init auto flag', () => { | |||
expect(stdout).not.toContain(firstPrompt); | |||
|
|||
// Test regressively files are scaffolded | |||
const files = ['./sw.js', './package.json', './yarn.lock', './src/index.js']; | |||
const files = ['./sw.js', './package.json', './src/index.js']; |
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.
This change is not needed now.
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.
Needed, these are still randomly failing.
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.
Then add a check for node_modules
folder, we need to check whether packages are installed or not.
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.
Done, I hope it stays consistent now.
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.
Tried adding node_modules, same behaviour, would need debugging later.
@@ -28,7 +28,7 @@ describe('init', () => { | |||
expect(stdout).toContain(firstPrompt); | |||
|
|||
// Test regressively files are scaffolded | |||
const files = ['./sw.js', './package.json', './yarn.lock', './src/index.js']; | |||
const files = ['./sw.js', './package.json', './src/index.js']; |
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.
Same here
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.
^ Same
test/plugin/plugin.test.js
Outdated
const pluginPath = join(__dirname, pluginName); | ||
|
||
// Since scaffolding is time consuming | ||
jest.setTimeout(200000); |
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.
Why number is so big?
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.
I'll reduce this, maybe I was testing
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.
Fixed @evilebottnawi
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.
/cc @webpack/cli-team
What kind of change does this PR introduce?
test
Did you add tests for your changes?
Yes
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information
Did some util changes in #1476 , this should go after that.