test: check for development mode and tests the correct execution of the dist #20
Conversation
gdorsi
changed the title
gdorsi
changed the title
test/cached-build.test.js
Outdated
| @@ -52,7 +53,7 @@ test('it should correctly generate all required pino files when the cache is ena | |||
| runBuild(distFolder, (err) => { | |||
| t.error(err) | |||
|
|
|||
| runBuild(distFolder, (err, stats) => { | |||
| runBuild(distFolder, async (err, stats) => { | |||
There was a problem hiding this comment.
never mix callbacks and async. Refactor this to just use promises if you think that's necessary.
There was a problem hiding this comment.
I've removed the async and changed the await parts to .then
Is it ok now?
test/development-mode.test.js
Outdated
| }) | ||
| }) | ||
|
|
||
| runBuild(distFolder, async (err, stats) => { |
There was a problem hiding this comment.
don't mix promises and callbacks.
test/index.test.js
Outdated
| @@ -36,7 +37,7 @@ test('it should correctly generated all required pino files', (t) => { | |||
| minimize: false | |||
| } | |||
| }, | |||
| (err, stats) => { | |||
| async (err, stats) => { | |||
There was a problem hiding this comment.
don't mix promises and callbacks.
| execa(process.argv[0], [secondDistFilePath]).then(({ stdout }) => { | ||
| t.match(stdout, /This is second!/) | ||
| }) |
There was a problem hiding this comment.
let's use async/await for a more modern look of this code :) here and elsewhere please
There was a problem hiding this comment.
With async/await the error management would become different.
There was a problem hiding this comment.
There is no error management you're doing
There was a problem hiding this comment.
Not mine but webpack's
I tried and in both cases when I throw an error inside the onDone callback the result would be an unhandledRejection so no difference here.
Anyway I think that "don't mix promises with callbacks" is a more general rule useful to avoid unintended and difficult to predict behavoirs.
/cc @mcollina
test/development-mode.test.js
Outdated
| test('should work in webpack development mode', (t) => { | ||
| t.plan(3) | ||
|
|
||
| const distFolder = resolve(__dirname, '../tmp/pino-transport-bug') |
There was a problem hiding this comment.
why is this folder called "pino-transport-bug"?
test/cached-build.test.js
Outdated
| @@ -39,7 +40,7 @@ function runBuild(distFolder, onDone) { | |||
| } | |||
|
|
|||
| test('it should correctly generate all required pino files when the cache is enabled on Webpack', (t) => { | |||
| t.plan(7) | |||
| t.plan(9) | |||
|
|
|||
| const distFolder = resolve(__dirname, '../tmp/cached-build') | |||
There was a problem hiding this comment.
why don't we use tap's testdir feature so we don't have to come up with fancy names for folders?
There was a problem hiding this comment.
Because i didn't know of it's existence :)
Thanks for pointing this out!
No description provided.