-
Notifications
You must be signed in to change notification settings - Fork 92
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
Add expectAssertions option to test #103
Conversation
I wasn't sure what to actually name the option, and I'd actually prefer to be able to call t.expectAssertions(2) instead, but that seemed hackish and more complex to implement. |
if you pass it to the assertion object then Semantically it is a bit in between: yes it is a kind of meta option of the test and yes it can be seen as an assertion on the test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the great effort.
I made some suggestions.
All together, I am not sure it is a worthwhile feature but we can keep on with the discussion in the issue thread
@@ -6,7 +6,7 @@ import { | |||
testEndMessage, | |||
} from 'zora-reporters'; | |||
|
|||
const defaultOptions = Object.freeze({ skip: false }); | |||
const defaultOptions = Object.freeze({ skip: false, expectAssertions: undefined }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so expectedAssertionCount
would be more appropriate.
I would not set it in the default options, I would simply omit it
if (expectAssertions !== undefined && assertions.length !== expectAssertions) { | ||
yield assertionMessage(Assert.equal(assertions.length, expectAssertions, 'expectAssertions')); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is not a good idea to use the regular equal operator. Creating a specific operator would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and instead of adding a message in the stream. you can simply add an assertion result at the end of the routine so it will be considered as a regular assertion:
const createAssertionCountResult = ({ expected, actual }) => ({
pass: expected === actual,
operator: 'plan',
expected,
actual,
description: 'assertion count should match',
});
export const test = (description, spec, opts = defaultOptions) => {
const { skip = false, expectAssertions = undefined } = opts;
/* ... */
const testRoutine = (async function () {
try {
const start = Date.now();
const result = await specFn();
executionTime = Date.now() - start;
// add an extra assertion if needed
if (expectAssertions) {
onResult(
createAssertionCountResult({
expected: expectAssertions,
actual: assertions.length,
})
);
}
return result;
} catch (e) {
error = e;
} finally {
done = true;
}
})();
return Object.assign(testRoutine, {
[Symbol.asyncIterator]: async function* () {
// no need to yield a specific message here
/* ... */
},
});
};
and if you wish to have add the method on the assertion API instead, you could do something like:
export const test = (description, spec, opts = defaultOptions) => {
const { skip = false } = opts;
const assertions = [];
let executionTime;
let done = false;
let error;
let plan;
/* ... */
const specFn = skip
? noop
: function zora_spec_fn() {
return spec(
Object.assign(assertFactory({ onResult }), {
// extend the API here
plan(value) {
plan = value;
},
})
);
};
const testRoutine = (async function () {
try {
const start = Date.now();
const result = await specFn();
executionTime = Date.now() - start;
// add an extra assertion if needed
if (plan) {
onResult(
createAssertionCountResult({
expected: plan,
actual: assertions.length,
})
);
}
return result;
} catch (e) {
error = e;
} finally {
done = true;
}
})();
return Object.assign(testRoutine, {
[Symbol.asyncIterator]: async function* () {
/* ... same here */
},
});
};
Upon discussion in #102 , I no longer believe this is necessary. If it comes up again, this will be a good starting poing for implementing the logic though. |
fixes #102