tests: fix folder structure, add more default tests #1258
Conversation
webpack-bot
added
PR: unreviewed
PR: CI-ok
PR: CI-not-ok
and removed
PR: CI-ok
PR: CI-not-ok
labels
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
anshumanv
changed the title
|
@ematipico seems good to go, can you think of any other cases that I can cover? |
| const { run, extractSummary } = require('../../utils/test-utils'); | ||
|
|
||
| describe('output flag defaults with config', () => { | ||
| it('should use default entry if config entry file is not present', done => { |
There was a problem hiding this comment.
I don't understand this test. Here there is a config file, so why the test says "not present"?
There was a problem hiding this comment.
The entry file specified in the config doesn't exist, so it will pick the default entry file ./index.js.
Should I improve the wording here?
There was a problem hiding this comment.
Maybe .. if entry file from config is not present ?
There was a problem hiding this comment.
Yes, we should improve the wording: "should override the [entry] property of the config file" We don't care if the file really exists or not
There was a problem hiding this comment.
Actually, if the entry file exists, it picks up the entry specified in the config instead of the default entry but in case it doesn't then it falls back to using the default entry
There was a problem hiding this comment.
This should NOT be the case. The point of enforcing defaults is to use OUR defaults and override everything that already exists.
There was a problem hiding this comment.
Which means there's a bug in our strategy when we apply the defaults
There was a problem hiding this comment.
Yes, will take a look at that too. 👍
There was a problem hiding this comment.
Skipping only this test for now, will enable it once I fix the above problem.
| const summary = extractSummary(stdout); | ||
| // Should use the output dir specified in the config | ||
| const outputDir = 'with-config-and-entry/binary'; | ||
| // eslint-disable-next-line quotes |
| const { stdout, stderr } = run(__dirname, ['--defaults'], false); | ||
| const summary = extractSummary(stdout); | ||
| const outputDir = 'without-config-and-entry/dist'; | ||
| // eslint-disable-next-line quotes |
There was a problem hiding this comment.
Remove this line. Use the template literal (`)
There was a problem hiding this comment.
Actually we're enforcing single quotes in eslint config.
|
And what's |
Ah, I meant to put architecture, but yes it should be removed good catch! 😄 |
anshumanv
changed the title
|
@anshumanv Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ematipico Please review the new changes. |
|
@ematipico, I'm getting a huge change on running watch command, these files aren't ignored in the respective .gitignore of their packages, should I modify the configs to ignore transpiled stuff too?
|
anshumanv
force-pushed
the
test/defaults
branch
from
053de25 to
38be92d
Compare
|
It could files of the old structure. When I was working on this refactor, I had to build manually delete all the files. Have you tried to delete those files and build again? The project is already setup correctly, where we have the src folder and the lib folder as destination where compiled files are stored and ignored in git. |
anshumanv
force-pushed
the
test/defaults
branch
from
4388552 to
25ec60a
Compare
Ah yes, not seeing them anymore, thanks @ematipico! Anything else that I need to take care of here? Seems good from my side. |
3 participants
What kind of change does this PR introduce?
Fix test arch, add more default case tests
Did you add tests for your changes?
Yay
If relevant, did you update the documentation?
NA
Summary
Does this PR introduce a breaking change?
No
Other information
Follow up of #1254