test: refactor mkdtemp test and added async #12080

lucamaraschi · 2017-03-27T22:07:17Z

This test refactored the original test for mkdtempSync prefix validation
and added the test also for the async function mkdtemp.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows [commit guidelines][]

Affected core subsystem(s)

test fs

lpinca · 2017-03-28T06:11:12Z

test/parallel/test-fs-mkdtemp-prefix-check.js

+
+function failAsync(value) {
+  assert.throws(
+    () => fs.mkdtemp(value, () => {}),


Nit: there is common.noop, so I think it makes sense to use it.

hiroppy · 2017-03-28T13:41:43Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7072/

richardlau

I'd prefer the variables are renamed. As they currently are they sound like they refer to the values that occurred during execution and are being tested rather than the expected error or test inputs.

richardlau · 2017-03-28T14:24:09Z

test/parallel/test-fs-mkdtemp-prefix-check.js

+const assert = require('assert');
+const fs = require('fs');
+
+const assertedError = /^TypeError: filename prefix is required$/;


Could this be expectedError?

richardlau · 2017-03-28T14:27:01Z

test/parallel/test-fs-mkdtemp-prefix-check.js

+const fs = require('fs');
+
+const assertedError = /^TypeError: filename prefix is required$/;
+const assertValues = [undefined, null, 0, true, false, 1];


prefixValues?

Fishrock123 · 2017-03-28T15:15:07Z

test/parallel/test-fs-mkdtemp-prefix-check.js

+
+function failAsync(value) {
+  assert.throws(
+    () => fs.mkdtemp(value, common.noop),


Should the callback be be () => common.fail('Async callback should not be called')?

That could work!

On second thought, I'm not sure that can happen because of the throw, but it would be good insurance anyways imo

Fishrock123 · 2017-03-28T15:16:01Z

Oops, didn't mean to approve. Oh well lol

richardlau · 2017-03-28T17:49:03Z

test/parallel/test-fs-mkdtemp-prefix-check.js

+function failAsync(value) {
+  assert.throws(
+    () => fs.mkdtemp(value, () => {
+      common.fail('Async callback should not be called');


I think this can be common.mustNotCall()

Semantically the two have the same result. We should probably start defining a list of guidelines to use (or maybe I just missed it) for tests.
@cjihrig I guess common.mustNotCall() should be the right pick here, I am right?

common.mustNotCall() was added to prevent calling common.fail() in an otherwise empty function.

This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp.

jasnell · 2017-03-30T05:24:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7100/

jasnell · 2017-04-04T17:19:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7201/

This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp. PR-URL: #12080 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

jasnell · 2017-04-04T18:51:27Z

Landed in 085c1f8

This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp. PR-URL: nodejs#12080 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins · 2017-04-18T23:40:42Z

This test does not pass on LTS. would you be willing to backport to v6.x-staging?

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 27, 2017

jasnell approved these changes Mar 27, 2017

View reviewed changes

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 27, 2017

cjihrig approved these changes Mar 27, 2017

View reviewed changes

lpinca reviewed Mar 28, 2017

View reviewed changes

lpinca approved these changes Mar 28, 2017

View reviewed changes

lucamaraschi force-pushed the test-mkdtemp-async-prefix-check branch from 6c6690f to 278d515 Compare March 28, 2017 12:18

hiroppy approved these changes Mar 28, 2017

View reviewed changes

richardlau requested changes Mar 28, 2017

View reviewed changes

lucamaraschi force-pushed the test-mkdtemp-async-prefix-check branch from 278d515 to 1323058 Compare March 28, 2017 15:09

richardlau approved these changes Mar 28, 2017

View reviewed changes

Fishrock123 approved these changes Mar 28, 2017

View reviewed changes

lucamaraschi force-pushed the test-mkdtemp-async-prefix-check branch from 1323058 to b83338c Compare March 28, 2017 15:29

richardlau reviewed Mar 28, 2017

View reviewed changes

lucamaraschi force-pushed the test-mkdtemp-async-prefix-check branch from b83338c to 12cc305 Compare March 28, 2017 20:55

test: refactor mkdtemp test and added async

daa39cd

This test refactored the original test for mkdtempSync prefix validation and added the test also for the async function mkdtemp.

lucamaraschi force-pushed the test-mkdtemp-async-prefix-check branch from 12cc305 to daa39cd Compare March 28, 2017 21:04

jasnell closed this Apr 4, 2017

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

italoacasas mentioned this pull request Apr 10, 2017

v7.9.0 Release Proposal #12319

Merged

2 tasks

MylesBorins added the backport-requested-v6.x label Apr 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: refactor mkdtemp test and added async #12080

test: refactor mkdtemp test and added async #12080

lucamaraschi commented Mar 27, 2017 •

edited

Loading

lpinca Mar 28, 2017 •

edited

Loading

hiroppy commented Mar 28, 2017

richardlau left a comment

richardlau Mar 28, 2017

richardlau Mar 28, 2017

Fishrock123 Mar 28, 2017

lucamaraschi Mar 28, 2017

Fishrock123 Mar 28, 2017

Fishrock123 commented Mar 28, 2017

richardlau Mar 28, 2017

lucamaraschi Mar 28, 2017

cjihrig Mar 28, 2017

jasnell commented Mar 30, 2017

jasnell commented Apr 4, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017 •

edited

Loading

test: refactor mkdtemp test and added async #12080

test: refactor mkdtemp test and added async #12080

Conversation

lucamaraschi commented Mar 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

lpinca Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

hiroppy commented Mar 28, 2017

richardlau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Mar 30, 2017

jasnell commented Apr 4, 2017

jasnell commented Apr 4, 2017

MylesBorins commented Apr 18, 2017 • edited Loading

lucamaraschi commented Mar 27, 2017 •

edited

Loading

lpinca Mar 28, 2017 •

edited

Loading

MylesBorins commented Apr 18, 2017 •

edited

Loading