Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: rename regression tests with descriptive file names #19212

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
'use strict';
const common = require('../common');

// Check that cluster works perfectly for both `kill` and `disconnect` cases.
// Also take into account that the `disconnect` event may be received after the
// `exit` event.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Maybe add a link to the GH issue that this test was named for originally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I could have missed this, will do.


const assert = require('assert');
const cluster = require('cluster');

Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
/* eslint-disable strict */
require('../common');
const assert = require('assert');

/*
In Node.js 0.10, a bug existed that caused strict functions to not capture
their environment when evaluated. When run in 0.10 `test()` fails with a
`ReferenceError`. See https://github.com/nodejs/node/issues/2245 for details.
*/
// In Node.js 0.10, a bug existed that caused strict functions to not capture
// their environment when evaluated. When run in 0.10 `test()` fails with a
// `ReferenceError`. See https://github.com/nodejs/node/issues/2245 for details.

const assert = require('assert');

function test() {

Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
'use strict';

const common = require('../common');
const tmpdir = require('../common/tmpdir');

// This test ensures that fs.existsSync doesn't incorrectly return false
// (especially on Windows)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It would be nice to punctuate the new comments.

// https://github.com/nodejs/node-v0.x-archive/issues/3739

const assert = require('assert');
const fs = require('fs');
const path = require('path');

const tmpdir = require('../common/tmpdir');

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I personally would not mode this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that in conflict with the canonical test structure?

Ref: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#lines-1-3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to the require('../common'); but it is fine to keep it as is.

let dir = path.resolve(tmpdir.path);

// Make sure that the tmp directory is clean
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
'use strict';
const common = require('../common');

// This test ensures fs.realpathSync works on properly on Windows without
// throwing ENOENT when the path involves a fileserver
// https://github.com/nodejs/node-v0.x-archive/issues/3542

// This test is only relevant on Windows.
if (!common.isWindows)
common.skip('Windows specific test.');

const assert = require('assert');
const fs = require('fs');
const path = require('path');

if (!common.isWindows)
common.skip('Windows specific test.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't think this should be moved. I think it should be left where it was.


function test(p) {
const result = fs.realpathSync(p);
assert.strictEqual(result.toLowerCase(), path.resolve(p).toLowerCase());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const common = require('../common');
const fixtures = require('../common/fixtures');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't think this should be moved either. I think it should be left where it was.


// This test ensures that a http request callback is called
// when the agent option is set
Expand All @@ -8,8 +9,6 @@ const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const fixtures = require('../common/fixtures');

const https = require('https');

const options = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@

'use strict';
require('../common');

// This test ensures that setting `process.domain` to `null` does not result in
// node crashing with a segfault
// https://github.com/nodejs/node-v0.x-archive/issues/4256

process.domain = null;
setTimeout(function() {
console.log('this console.log statement should not make node crash');
Expand Down