WIP: Build and run tests with webpack#770
WIP: Build and run tests with webpack#770bcheidemann wants to merge 14 commits intofilerjs:masterfrom
Conversation
|
@humphd Is this the kind of thing you were talking about? |
Codecov Report
@@ Coverage Diff @@
## master #770 +/- ##
==========================================
+ Coverage 86.89% 87.06% +0.16%
==========================================
Files 16 20 +4
Lines 1740 1770 +30
==========================================
+ Hits 1512 1541 +29
- Misses 228 229 +1
Continue to review full report at Codecov.
|
package.json
Outdated
| "es6-promisify": "^6.1.0", | ||
| "minimatch": "^3.0.4" | ||
| "minimatch": "^3.0.4", | ||
| "node-polyfill-webpack-plugin": "^1.0.3", |
There was a problem hiding this comment.
These additional deps look like they can move to devDependencies
There was a problem hiding this comment.
Yeah, I'll move those over 👍
| @@ -4,7 +4,7 @@ const bufferDefault = require('../../../shims/buffer').default; | |||
| const bufferNamed = require('../../../shims/buffer').Buffer; | |||
There was a problem hiding this comment.
Let's rebase on master to pick up these changes there vs. duplicating.
| entry: path.resolve(__dirname, './webpack-tests.js'), | ||
| resolve: { | ||
| alias: { | ||
| 'fsProvider': path.resolve(__dirname, '../shims/providers/default'), |
There was a problem hiding this comment.
Forgive my webpack ignorance here: can we not use Filer in this setup the way you're outlining in the README? It would be great if we had test code that did what the README suggests, so we know if it breaks.
There was a problem hiding this comment.
Do you mean aliasing fs, path and Buffer? We can definitely do that but obviously we'd need to refactor the shim tests to require fs, path and Buffer instead of the shims so those tests would have to be separated from the rest of the tests. I think that makes sense though as they're webpack specific.
There was a problem hiding this comment.
Right, that's what I was thinking. Given that there are a few ways to do this, having adequate test coverage of all of them is my goal.
There was a problem hiding this comment.
Hey @humphd, I'm still working on this PR but found an issue with the way we're shimming the path module. In short, the alias messes up the import of the path module in filer (which I'd previously overlooked) and as such the path module ends up missing a lot of the methods (all the ones not explicitly replaced by filer). It should be possible to write a webpack plugin which resolves the path module correctly. Are you happy for me to go ahead with this and to make a separate PR for it?
There was a problem hiding this comment.
@bcheidemann I think that sounds like a good idea. Thanks for pushing on this code a bit, it's great that testing is already discovering ways we can make it better.
There was a problem hiding this comment.
No problem! I'm happy to do it and it's teaching me a lot :D I just wish I had more time to work on it at the moment!
This was done so the sister webpack build can use the same mocha setup script
bcheidemann
force-pushed
the
webpackTestBuild
branch
from
05ea9e7 to
5857f1a
Compare
bcheidemann
changed the title
|
Ping me when you want reviews on these. |
|
No problem 👍 |
|
I think doing it in this repo is probably the best idea, since it has CI setup already, and is so closely related to the other code. I'm more wondering about reducing the bundle size for Filer for cases where people aren't using the webpack plugin. We could write a separate set of build/test scripts to be run with |
humphd
left a comment
humphd
left a comment
There was a problem hiding this comment.
One nit and the lock file needs to be regenerated, otherwise looks good.
| }, | ||
| "dependencies": { | ||
| "es6-promisify": "^6.1.0", | ||
| "path": "^0.12.7", |
There was a problem hiding this comment.
Remove this, it's a node built-in module.
3 participants
Adds the option to build and run tests with webpack using the command
npm run test:webpack. (Similar tonpm run test:manualin that it spins up a local server)