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.

Sign up for GitHub

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: split up and refactor test-domain #13614

Closed
wants to merge 1 commit into from
Closed

test: split up and refactor test-domain #13614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions test/parallel/test-domain-bind-timeout.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

const d = new domain.Domain();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, undefined);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, true);
}));

setTimeout(d.bind(() => { throw new Error('foobar'); }), 1);
28 changes: 28 additions & 0 deletions test/parallel/test-domain-ee-implicit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const EventEmitter = require('events');

const d = new domain.Domain();
let implicit;

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, implicit);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, false);
}));

// Implicit addition of the EventEmitter by being created within a domain-bound
// context.
d.run(common.mustCall(() => {
implicit = new EventEmitter();
}));

setTimeout(common.mustCall(() => {
// escape from the domain, but implicit is still bound to it.
implicit.emit('error', new Error('foobar'));
}), 1);
20 changes: 20 additions & 0 deletions test/parallel/test-domain-ee.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const EventEmitter = require('events');

const d = new domain.Domain();
const e = new EventEmitter();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, e);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, false);
}));

d.add(e);
e.emit('error', new Error('foobar'));
23 changes: 23 additions & 0 deletions test/parallel/test-domain-error-types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// This test is similar to test-domain-multiple-errors, but uses a new domain
// for each errors.

for (const something of [
42, null, undefined, false, () => {}, 'string', Symbol('foo')
]) {
const d = new domain.Domain();
d.run(common.mustCall(() => {
process.nextTick(common.mustCall(() => {
throw something;
}));
}));

d.on('error', common.mustCall((err) => {
assert.strictEqual(something, err);
}));
}
20 changes: 20 additions & 0 deletions test/parallel/test-domain-fs-enoent-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const fs = require('fs');

const d = new domain.Domain();

const fst = fs.createReadStream('stream for nonexistent file');

d.on('error', common.mustCall((err) => {
assert.ok(err.message.match(/^ENOENT: no such file or directory, open '/));
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, fst);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, false);
}));

d.add(fst);
31 changes: 31 additions & 0 deletions test/parallel/test-domain-implicit-binding.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const fs = require('fs');

{
const d = new domain.Domain();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, undefined);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, true);
}));

d.run(common.mustCall(() => {
process.nextTick(common.mustCall(() => {
const i = setInterval(common.mustCall(() => {
clearInterval(i);
setTimeout(common.mustCall(() => {
fs.stat('this file does not exist', common.mustCall((er, stat) => {
throw new Error('foobar');
}));
}), 1);
}), 1);
}));
}));
}
43 changes: 43 additions & 0 deletions test/parallel/test-domain-intercept.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

{
const d = new domain.Domain();

const mustNotCall = common.mustNotCall();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, undefined);
assert.strictEqual(err.domainBound, mustNotCall);
assert.strictEqual(err.domainThrown, false);
}));

const bound = d.intercept(mustNotCall);
bound(new Error('foobar'));
}

{
const d = new domain.Domain();

const bound = d.intercept(common.mustCall((data) => {
Copy link

Choose a reason for hiding this comment

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

Instead of removing them, keeping (or improving on) the comments present in the previous version of this test would help understand it.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem I see with the previous comments regarding this test (and some of the others) is that they seemed more like API documentation rather than actually helping to explain what the test does. In this case, I hope it becomes much more visible what each part of the test does by identifying the groups of code that test a single piece of the functionality.

Copy link

Choose a reason for hiding this comment

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

Some of these comments describe the intention of these tests. For instance:

// catch thrown errors no matter how many times we enter the event loop [...]

Looking at the new version of that test in test-domain-implicit-binding.js, it's not clear why the error is thrown from a nested async operation nested several layers deep, and so it's not clear to the person who hasn't written this test whether it even makes sense to keep this test.

I still think it would be worth it to keep those comments.

assert.strictEqual(data, 'data');
}));

bound(null, 'data');
}

{
const d = new domain.Domain();

const bound = d.intercept(common.mustCall((data, data2) => {
assert.strictEqual(data, 'data');
assert.strictEqual(data2, 'data2');
}));

bound(null, 'data', 'data2');
}
26 changes: 26 additions & 0 deletions test/parallel/test-domain-multiple-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
'use strict';
Copy link

Choose a reason for hiding this comment

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

What is the difference between this test and test/parallel/test-domain-error-types.js?

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference is that one of the tests emits the errors on the same domain object, because that was requested here. I’ve added comments referencing the other test file in each of them.

Copy link

Choose a reason for hiding this comment

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

Great, thanks!


const common = require('../common');
const assert = require('assert');
const domain = require('domain');

// This test is similar to test-domain-error-types, but uses a single domain
// to emit all errors.

const d = new domain.Domain();

const values = [
42, null, undefined, false, () => {}, 'string', Symbol('foo')
];

d.on('error', common.mustCall((err) => {
assert(values.includes(err));
}, values.length));

for (const something of values) {
d.run(common.mustCall(() => {
process.nextTick(common.mustCall(() => {
throw something;
}));
}));
}
21 changes: 21 additions & 0 deletions test/parallel/test-domain-nexttick.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

const d = new domain.Domain();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, undefined);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, true);
}));

d.run(common.mustCall(() => {
process.nextTick(common.mustCall(() => {
throw new Error('foobar');
}));
}));
13 changes: 13 additions & 0 deletions test/parallel/test-domain-run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
'use strict';

require('../common');
const assert = require('assert');
const domain = require('domain');

const d = new domain.Domain();

assert.strictEqual(d.run(() => 'return value'),
Copy link

Choose a reason for hiding this comment

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

Here too, keeping or improving on the comments of the previous version of this test would help understand its purpose.

'return value');

assert.strictEqual(d.run((a, b) => `${a} ${b}`, 'return', 'value'),
'return value');
21 changes: 21 additions & 0 deletions test/parallel/test-domain-timer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const domain = require('domain');

const d = new domain.Domain();

d.on('error', common.mustCall((err) => {
assert.strictEqual(err.message, 'foobar');
assert.strictEqual(err.domain, d);
assert.strictEqual(err.domainEmitter, undefined);
assert.strictEqual(err.domainBound, undefined);
assert.strictEqual(err.domainThrown, true);
}));

d.run(common.mustCall(() => {
setTimeout(common.mustCall(() => {
throw new Error('foobar');
}), 1);
}));
Loading