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

Adjust ValueAssertTest for changed format of mismatched string exception #212

Merged
merged 1 commit into from Feb 4, 2021

Conversation

mmathesius
Copy link
Contributor

This fix is required when building the xmlunit package for recent versions of Fedora.

It also corresponds to the exception message format that should be generated by the xmlunit code:

private static final String EXPECTED_BUT_WAS_MESSAGE = "%nExpecting:%n <%s>%nto be equal to:%n <%s>%nbut was not.";

However, running the tests directly using mvn clean test fail with this fix because the old format exception message is returned. (Unfortunately, I don't know Java well enough to be able to explain why!)

@bodewig
Copy link
Member

bodewig commented Feb 3, 2021

Actually running mvn clean test passes for the main branch while it fails if your pull request is applied (see https://travis-ci.com/github/xmlunit/xmlunit/jobs/479835608 for example).

The test is not related to the line you mention, it verifies the error message created by AssertJ's AbstractCharSequenceAssert (it is what this class' isEqualTo creates as message) and thus may be sensitive to the version of AssertJ used as a dependency. If you've changed the AssertJ version used when building the module, this could explain the test failure on your side.

@bodewig
Copy link
Member

bodewig commented Feb 3, 2021

I think assertj/assertj@5ae16a9 is responsible for the test failure. It went into AssertJ 3.19.0.

The xmlunit-assertj module explicitly wants to support AssertJ 2.x and we encourage users to use xmlunit-assertj3 if they want to use AssertJ 3.x instead.

I see a couple of options:

  1. you somehow manage to compile the different XMLUnit modules against different versions of AssertJ - this is what Maven does by default and why it works outside of your build environment.
  2. you drop the xmlunit-assertj module (and potentially make your users unhappy)
  3. we rewrite the test assertion so it works with both AssertJ 2.x and AssertJ 3.19.0+

most likely we'll end up with the last option but maybe I'm overlooking something. If we want to allow either message pattern, we should do so as strictly as possible. A test that only looked for the presense of substrings would miss bugs like #210.

@bodewig
Copy link
Member

bodewig commented Feb 3, 2021

please take a look at https://github.com/xmlunit/xmlunit/compare/212-prototype?expand=1 - if this works for you, I'll expand it to the assertj3 module and add a compat-test for AssertJ 3.19.0

@mmathesius
Copy link
Contributor Author

@bodewig Thanks for the feedback and improved patch! I checked your proposed patch with a Fedora build and it almost works. The %n references in the AssertJ 3.19.0+ pattern alternative simply need to be changed to \n and it's good.

Signed-off-by: Merlin Mathesius <mmathesi@redhat.com>
@bodewig
Copy link
Member

bodewig commented Feb 3, 2021

thanks. If you were building XMLUnit 2.8.x you'd get related issues for the xmlunit-assertj3 module, it looks as if I needed to adjust more patterns there.

@mmathesius
Copy link
Contributor Author

@bodewig I just updated this PR with the improved patch. The package in Fedora is still using 2.7.0, and this patch is sufficient for that. Thanks again!

@coveralls
Copy link

coveralls commented Feb 3, 2021

Coverage Status

Coverage remained the same at 91.948% when pulling 87796e0 on mmathesius:exception-format-update into 5b66f10 on xmlunit:main.

@bodewig bodewig merged commit f7d5a28 into xmlunit:main Feb 4, 2021
@bodewig
Copy link
Member

bodewig commented Feb 4, 2021

Many thanks

bodewig added a commit that referenced this pull request Feb 4, 2021
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.

None yet

3 participants