Skip to content

tests: add e2e tests for plugin#1478

Merged
anshumanv merged 38 commits into
webpack:nextfrom
anshumanv:test/plugin
May 13, 2020
Merged

tests: add e2e tests for plugin#1478
anshumanv merged 38 commits into
webpack:nextfrom
anshumanv:test/plugin

Conversation

@anshumanv

Copy link
Copy Markdown
Contributor

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

  • Add e2e tests for plugin

Does this PR introduce a breaking change?
No

Other information

  • Any other cases to be tested are appreciated.

Did some util changes in #1476 , this should go after that.

@anshumanv anshumanv closed this Apr 18, 2020
@anshumanv anshumanv reopened this Apr 18, 2020
@anshumanv
anshumanv marked this pull request as ready for review April 18, 2020 16:58
@anshumanv
anshumanv requested a review from a team as a code owner April 18, 2020 16:58
@anshumanv

Copy link
Copy Markdown
Contributor Author

Ahh, looking at tests

Comment thread packages/webpack-cli/__tests__/init/auto/init-auto.test.js
Comment thread test/loader/loader.test.js
@alexander-akait

Copy link
Copy Markdown
Member

@anshumanv strange, CI green

@anshumanv

anshumanv commented Apr 20, 2020

Copy link
Copy Markdown
Contributor Author

@evilebottnawi I removed checks for yarn.lock(from both init and loader as either of them fails if checked), fails otherwise 😭

@alexander-akait

Copy link
Copy Markdown
Member

@anshumanv problems only locally?

@anshumanv

Copy link
Copy Markdown
Contributor Author

@evilebottnawi only in MacOS CI, passes locally on MacOS

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

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.

@anshumanv

Copy link
Copy Markdown
Contributor Author

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. 👍

@webpack-bot

Copy link
Copy Markdown

@anshumanv Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@rishabh3112 Please review the new changes.

alexander-akait
alexander-akait previously approved these changes May 7, 2020

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

snitin315
snitin315 previously approved these changes May 7, 2020

@rishabh3112 rishabh3112 left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread test/init/auto/init-auto.test.js Outdated

// 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'];

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.


// 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'];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

^ Same

@anshumanv

Copy link
Copy Markdown
Contributor Author

/cc @evilebottnawi @rishabh3112

Comment thread test/plugin/plugin.test.js Outdated
const pluginPath = join(__dirname, pluginName);

// Since scaffolding is time consuming
jest.setTimeout(200000);

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/cc @webpack/cli-team

@anshumanv
anshumanv merged commit 6a8dd32 into webpack:next May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants