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

Experimentally disable null expectations for throws assertions #2576

Merged
merged 7 commits into from
Sep 9, 2020

Conversation

JSimoni42
Copy link
Contributor

Aims to fix #2410

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up @JSimoni42!

@@ -87,20 +87,29 @@ function getErrorWithLongStackTrace() {
return err;
}

function validateExpectations(assertion, expectations, numberArgs) { // 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.

@novemberborn
Copy link
Member

Awesome.

What's left now is to update the documentation:

In both those places it says you need to specify null. Since we now allow undefined as well let's just recommend that. And then we can say something in parentheses like:

(AVA 3 also allows you to specify null. This will be removed in AVA 4. You can opt in to this change early by enabling the disableNullExpectations experiment.)

@JSimoni42 JSimoni42 marked this pull request as ready for review September 7, 2020 16:07
@JSimoni42
Copy link
Contributor Author

@novemberborn Documentation has been updated. I noticed that #2575 was also failing checks due to the same test error in Install and test AVA / Node.js (^14.0.0, ubuntu-latest). Suffice it to say, it seems like this PR didn't introduce any additional test failures. Let me know if I should squash/merge or if you'd prefer to handle that, yourself

@novemberborn
Copy link
Member

Let me know if I should squash/merge or if you'd prefer to handle that, yourself

Should be fine now…

@novemberborn novemberborn changed the title Disallow null expectations Experimentally disable null expectations for throws assertions Sep 9, 2020
@novemberborn novemberborn merged commit f328a69 into avajs:master Sep 9, 2020
@novemberborn
Copy link
Member

Thanks @JSimoni42!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow null expectations for throws and throwsAsync assertions
2 participants