Skip to content

Changed failure message of StringSubject.matches and added test cases#789

Closed
alexw0610 wants to merge 2 commits into
google:masterfrom
alexw0610:matchesFailMessage
Closed

Changed failure message of StringSubject.matches and added test cases#789
alexw0610 wants to merge 2 commits into
google:masterfrom
alexw0610:matchesFailMessage

Conversation

@alexw0610

Copy link
Copy Markdown
Contributor

Added an additional message to hint at the use of isEqualTo in case of a failed test
where most likley a exact equality assertion was needed instead of a regex.

Added two test cases to StringSubjectTest to test for the correct failure messages.
This PR is in reference to #783 (comment)

Added an additional message to hint at the use of isEqualTo in case of a failed test
where most likley a exact equality assertion was needed instead of a regex.
Added two test cases to StringSubjectTest to test for the correct failure messages.
@google-cla google-cla Bot added the cla: yes label Nov 21, 2020
@alexw0610

Copy link
Copy Markdown
Contributor Author

Does it make sense to apply those same changes to the StringSubject.doesNotMatch method?
assertThat("$test").doesNotMatch("$test")
for example would pass even if it was not intended to by the user.
I can't think of an example where doesNotMatch would fail.
assertThat("test").doesNotMatch("test")
would fail intentionally.

@cpovirk

cpovirk commented Nov 23, 2020

Copy link
Copy Markdown
Member

Thanks.

RE: doesNotMatch: Looking at our internal users of doesNotMatch makes me so sad :( So many cases like doesNotMatch("^foo"), which checks only that the string does not exactly match "foo." That is, it does not just search for a match, but rather the whole string must match. But I agree that we can't do anything about that here.

One comment: It seems like users would be unlikely to run into this problem the Pattern overload: If a user writes assertThat(foo).matches(Pattern.compile(bar)), then it should be fairly clear that the user is requesting a regex match. I can see 3 courses of action:

  1. Still warn about it, as in your PR. After all, what's the harm?
  2. Warn about it, but suggest using matches(Pattern.quote(...)) instead of isEqualTo(...). My thinking is that someone might have wrapped matches in a helper method, something like fooMustMatch(String regex). If a user is stuck calling that method, then that user can't simply switch from matches to isEqualTo, since the call to matches happens inside the method. In that case, the fix would be to quote the expected string into regex form with Pattern.quote. (We do somewhat discourage such helper methods, for this and other reasons, but people do it, and we could help them out.)
  3. Add the warning for the String overload only.

Thoughts?

Changed the error message a user receives if a matches() test fails
with the Pattern overload where the literal representation of the Pattern
matches the test string.
@alexw0610

alexw0610 commented Nov 23, 2020

Copy link
Copy Markdown
Contributor Author

Yeah good catch. If someone uses the Pattern overload they very likely know they are checking with a regex. I think option 2 makes the most sense here.
I changed the error message to:
"If you want an exact equality assertion you can escape your regex with Pattern.quote()."

In any case that will give the user a hint since

expected to match: $foo
but was          : $foo

can be quite confusing at first.
Hope you are fine with that and thanks for reviewing the PR. 👍

@cpovirk cpovirk self-assigned this Nov 24, 2020
@cpovirk cpovirk added P2 has an ETA type=enhancement Make an existing feature better labels Nov 24, 2020
copybara-service Bot pushed a commit that referenced this pull request Nov 30, 2020
… string match for the actual value.

Fixes #789
Fixes #783

RELNOTES=n/a
PiperOrigin-RevId: 344063799
copybara-service Bot pushed a commit that referenced this pull request Nov 30, 2020
… string match for the actual value.

Fixes #789
Fixes #783

RELNOTES=n/a
PiperOrigin-RevId: 344063799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes P2 has an ETA type=enhancement Make an existing feature better

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants