fix: Upgraded project development environment to babel 7 and webpack 4 #1505
Conversation
rpl
force-pushed
the
update-deps/babel7-and-webpack4
branch
3 times, most recently
from
94af977
to
f3057f4
Compare
There was a problem hiding this comment.
Have you thought of using stubs instead of injecting fake implementations? The advantage is that the implementation is not filled with test-specific parameters. The disadvantage is that if the test somehow fails unexpectedly and restore isn't called, that other tests may fail in unexpected ways.
let stub = sinon.stub(fs, "readFile").callsFake(() => ... );
// or if you want to asynchronously throw:
let stub = sinon.stub(fs, "readFile").rejects(new Error(""));
// Done? Restore original
stub.restore();
| }; | ||
| }, | ||
| const FxProfile = {}; | ||
| FxProfile.Finder = function Finder() { |
There was a problem hiding this comment.
These tests has started to fail because of Finder wasn't considered a constructor, I think that the upgrade from babel 6 to babel 7 somehow unveiled the issue.
| throw new Error('Unexpected fs.readFile error'); | ||
| }); | ||
| await assert.isRejected( | ||
| getIdFromSourceDir('fakeSourceDir', fakeAsyncFsReadFile), |
There was a problem hiding this comment.
If you want to minimize the number of injected dependencies, then you could create a directory or file without the right permissions (since the the implementation doesn't handle the EACCES error). This makes the test a bit less artificial than injecting a fake fs.readFile.
There was a problem hiding this comment.
yeah, I was initially thinking of going for that strategy, but I would prefer to don't add this test to the ones skipped on windows (I would like to reduce the number of tests skipped on windows, not growing that list), and so I preferred to use this strategy instead.
rpl
force-pushed
the
update-deps/babel7-and-webpack4
branch
from
f3057f4
to
0e98fc2
Compare
@Rob--W yeah, obviously I've been also thinking about that, to be honest I would like to rework our unit tests to avoid all the "dependency injection" that we are currently using (almost everywhere in the project) just to make those tests possible, not just the ones added in this PR. But in this PR I really want to avoid any change that is not strictly necessary to complete the port to babel 7 and webpack 4, and so I just added the minimum set of additional tests needed to make coveralls happy again, using the testing strategy currently used in the rest of the project. In the meantime I filed it as #1511 to follow up on that. |
No description provided.