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

Ensure assertion message is a string #1831

Closed
wants to merge 1 commit into from
Closed

Ensure assertion message is a string #1831

wants to merge 1 commit into from

Conversation

TheDancingCode
Copy link
Contributor

This adds a typecheck for the optional assertion message parameter, ensuring it's a string if it is provided. Implementation is the same as in #1174, but this time with a test.

Fixes #1125.

@novemberborn
Copy link
Member

This only checks if the assertion fails. I think this should validate when the assertion is used, and fail the assertion when the message is not a string. Perhaps something like:

// …
const fail = callbacks.fail

const getMessage = (test, args) => {
  if (args.length === 0) {
    return ''
  }

  if (typeof args[0] !== 'string') {
    fail(test, new AssertionError({message: 'Assertion message must be a string', values: [formatWithLabel('Got:', args[0])]})
  }

  return args[0]
}

const assertions = {
  is(actual, expected, ...rest) {
    const message = getMessage(this, rest)
    // …
  }
}

Except that fail() doesn't throw so we need to be a little more creative to ensure we don't run the actual assertion if the message is the wrong type.

@TheDancingCode
Copy link
Contributor Author

TheDancingCode commented Jun 12, 2018

I took this on because I thought it was a rather easy issue, but I'm afraid it's a bit over my head. I don't understand the structure of AVA well enough and thus lack what it takes to be "a litte more creative". Maybe we should close this and let someone else try.

Sorry for that.

@novemberborn
Copy link
Member

Hey no worries @TheDancingCode. Apologies for writing "a little creative", I didn't mean to imply it was easy: I don't really know what the answer is either! You can close this if you want, or we can figure out what the right solution is (I'm going to be mostly away the next couple of weeks though).

Do you agree that we should fail the assertion when a non-string message is provided, even if it would have otherwise succeeded?

@TheDancingCode
Copy link
Contributor Author

I guess so.

I'll close this for now. Who knows, if inspiration strikes, I'll get back to this again. ;)

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.

2 participants