Skip to content

Experimentally disable null expectations for throws assertions #2576

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

Merged
merged 7 commits into from
Sep 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions docs/03-assertions.md
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ Assert that an error is thrown. `fn` must be a function which should throw. The
* `name`: the expected `.name` value of the thrown error
* `code`: the expected `.code` value of the thrown error

`expectation` does not need to be specified. If you don't need it but do want to set an assertion message you have to specify `null`.
`expectation` does not need to be specified. If you don't need it but do want to set an assertion message you have to specify `undefined`. (AVA 3 also allows you to specify `null`. This will be removed in AVA 4. You can opt into this change early by enabling the `disableNullExpectations` experiment.)

Example:

Expand Down Expand Up @@ -276,7 +276,7 @@ The thrown value *must* be an error. It is returned so you can run more assertio
* `name`: the expected `.name` value of the thrown error
* `code`: the expected `.code` value of the thrown error

`expectation` does not need to be specified. If you don't need it but do want to set an assertion message you have to specify `null`.
`expectation` does not need to be specified. If you don't need it but do want to set an assertion message you have to specify `undefined`. (AVA 3 also allows you to specify `null`. This will be removed in AVA 4. You can opt into this change early by enabling the `disableNullExpectations` experiment.)

Example:

Expand Down
14 changes: 11 additions & 3 deletions lib/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,16 @@ function getErrorWithLongStackTrace() {
return err;
}

function validateExpectations(assertion, expectations, numberArgs) { // eslint-disable-line complexity
function validateExpectations(assertion, expectations, numberArgs, experiments) { // eslint-disable-line complexity
if (numberArgs === 1 || expectations === null || expectations === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

I think it'll be better if we keep this clause, but then before assigning expectations = {} we check experiments.disableNullExpectations && expectations === null. It's OK to duplicate the code to throw the assertion error.

The easier it is to understand the experimental logic, the easier to upgrade the codebase to make it the default logic.

if (experiments.disableNullExpectations && expectations === null) {
throw new AssertionError({
assertion,
message: `The second argument to \`t.${assertion}()\` must be an expectation object or \`undefined\``,
values: [formatWithLabel('Called with:', expectations)]
});
}

expectations = {};
} else if (
typeof expectations === 'function' ||
Expand Down Expand Up @@ -465,7 +473,7 @@ class Assertions {
}

try {
expectations = validateExpectations('throws', expectations, args.length);
expectations = validateExpectations('throws', expectations, args.length, experiments);
} catch (error) {
fail(error);
return;
Expand Down Expand Up @@ -531,7 +539,7 @@ class Assertions {
}

try {
expectations = validateExpectations('throwsAsync', expectations, args.length);
expectations = validateExpectations('throwsAsync', expectations, args.length, experiments);
} catch (error) {
fail(error);
return Promise.resolve();
Expand Down
2 changes: 1 addition & 1 deletion lib/load-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const pkgConf = require('pkg-conf');

const NO_SUCH_FILE = Symbol('no ava.config.js file');
const MISSING_DEFAULT_EXPORT = Symbol('missing default export');
const EXPERIMENTS = new Set(['configurableModuleFormat', 'disableSnapshotsInHooks', 'reverseTeardowns']);
const EXPERIMENTS = new Set(['configurableModuleFormat', 'disableNullExpectations', 'disableSnapshotsInHooks', 'reverseTeardowns']);

// *Very* rudimentary support for loading ava.config.js files containing an `export default` statement.
const evaluateJsConfig = configFile => {
Expand Down
34 changes: 32 additions & 2 deletions test-tap/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const HelloMessage = require('./fixture/hello-message');
let lastFailure = null;
let lastPassed = false;

const assertions = new class extends assert.Assertions {
const AssertionsBase = class extends assert.Assertions {
constructor(overwrites = {}) {
super({
pass: () => {
Expand All @@ -35,7 +35,9 @@ const assertions = new class extends assert.Assertions {
...overwrites
});
}
}();
};

const assertions = new AssertionsBase();

function assertFailure(t, subset) {
if (!lastFailure) {
Expand Down Expand Up @@ -1474,6 +1476,34 @@ test('.throwsAsync() fails if passed a bad expectation', t => {
t.end();
});

test('.throws() fails if passed null expectation with disableNullExpectations', t => {
const asserter = new AssertionsBase({experiments: {disableNullExpectations: true}});

failsWith(t, () => {
asserter.throws(() => {}, null);
}, {
assertion: 'throws',
message: 'The second argument to `t.throws()` must be an expectation object or `undefined`',
values: [{label: 'Called with:', formatted: /null/}]
});

t.end();
});

test('.throwsAsync() fails if passed null expectation with disableNullExpectations', t => {
const asserter = new AssertionsBase({experiments: {disableNullExpectations: true}});

failsWith(t, () => {
asserter.throwsAsync(() => {}, null);
}, {
assertion: 'throwsAsync',
message: 'The second argument to `t.throwsAsync()` must be an expectation object or `undefined`',
values: [{label: 'Called with:', formatted: /null/}]
});

t.end();
});

test('.notThrows()', gather(t => {
// Passes because the function doesn't throw
passes(t, () => {
Expand Down