Skip to content
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

test: default loader name and plugin name #2392

Merged
merged 4 commits into from
Feb 2, 2021

Conversation

rishabh3112
Copy link
Member

What kind of change does this PR introduce?
feat

Did you add tests for your changes?
Not needed, already present

If relevant, did you update the documentation?
No

Summary
Enforced use of process.cwd() as default output-path

Does this PR introduce a breaking change?
No

Other information
Part of Roadmap

@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #2392 (dd95a1d) into master (7bd747a) will increase coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
+ Coverage   71.27%   71.74%   +0.46%     
==========================================
  Files          46       47       +1     
  Lines        2155     2173      +18     
  Branches      568      573       +5     
==========================================
+ Hits         1536     1559      +23     
+ Misses        619      614       -5     
Impacted Files Coverage Δ
packages/webpack-cli/lib/bootstrap.js 100.00% <0.00%> (ø)
...ges/webpack-cli/lib/utils/dynamic-import-loader.js 83.33% <0.00%> (ø)
packages/webpack-cli/lib/webpack-cli.js 91.88% <0.00%> (+0.80%) ⬆️
packages/webpack-cli/lib/utils/index.js 84.00% <0.00%> (+1.39%) ⬆️
packages/webpack-cli/bin/cli.js 50.00% <0.00%> (+5.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bd747a...dd95a1d. Read the comment docs.

Copy link
Member

@jamesgeorge007 jamesgeorge007 left a comment

Choose a reason for hiding this comment

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

@alexander-akait
Copy link
Member

@jamesgeorge007 Good catch, maybe we just need add test(s)

@rishabh3112
Copy link
Member Author

Good catch, maybe we just need add test(s)

Tests are already present. Why this was included in roadmap?

@alexander-akait
Copy link
Member

Can you provide link on tests? Maybe I something missed

@alexander-akait
Copy link
Member

alexander-akait commented Feb 1, 2021

No tests, https://github.com/webpack/webpack-cli/blob/master/test/loader/loader.test.js#L22 is not enough for testing, it is stupid test on the question, it should be fully test with checking on files https://github.com/webpack/webpack-cli/blob/master/test/loader/loader.test.js#L43

@rishabh3112
Copy link
Member Author

Line 29 has it.

@alexander-akait
Copy link
Member

Line 29 pass loaderName - let { stdout } = await runPromptWithAnswers(__dirname, ['loader'], [${loaderName}${ENTER}]);, we should have test without passing loader name (full test)

@rishabh3112
Copy link
Member Author

rishabh3112 commented Feb 1, 2021

Loader name is not being passed as output-path, it is an answer to an inquirer question asked by generator.

Nevertheless will add test for default loader name as well.

@rishabh3112 rishabh3112 changed the title feat: use process.cwd as default output-path test: default loader name and plugin name Feb 2, 2021
@rishabh3112 rishabh3112 force-pushed the feat/loader-plugin-output-path branch from af7b33d to dd95a1d Compare February 2, 2021 13:05
@alexander-akait alexander-akait merged commit 2841cd7 into master Feb 2, 2021
@alexander-akait alexander-akait deleted the feat/loader-plugin-output-path branch February 2, 2021 14:36
@alexander-akait
Copy link
Member

Thanks

This was referenced Mar 11, 2021
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