vitruv-tools / Vitruv

View-based Development and Model Consistency Framework
http://vitruv.tools
Eclipse Public License 1.0
14 stars 19 forks source link

Improve & Test Test Utils #429

Closed jGleitz closed 3 years ago

jGleitz commented 3 years ago

This PR adds unit tests for the most complex and critical parts of the testutils. Furthermore, it improves the model matcher significantly. Instead of fixing EMF Compare’s matches after the fact, we now get EMF Compare to generate correct matches in the first place.

Plus, there some small improvements to model printing.

HeikoKlare commented 3 years ago

Note: There seems to be an OS dependency in tools.vitruv.testutils.tests.matchers.ModelDeepEqualityMatcherTest.samefallbackIdInReporting. It fails on a Windows systems but seems to work fine on UNIX systems (as the build does not fail). I'll try to figure that out.

This simply seems to be an issue with different linebreaks.

HeikoKlare commented 3 years ago

I would propose to remove the formatting when comparing the message in the failing test case or is there any necessity to match their format as well?

jGleitz commented 3 years ago

I am not sure what you mean. What would ‘removing the formatting’ look like?

HeikoKlare commented 3 years ago

Sorry for the impreciseness. I mean something like matching the messages with replaceAll("\\s+","") to remove whitespace, line breaks and so on (we could also just remove line breaks and tabspaces for example). The message contains a mixture of different line breaks, of which some are system dependent while others are not, and I don't think that matching them is actually necessary.

jGleitz commented 3 years ago

I have no objections to removing whitespace before matching. However,

The message contains a mixture of different line breaks

sounds like a bug. So I’d prefer fixing the bug instead of changing the tests to work around the bug.

jGleitz commented 3 years ago

If you post which line delimiters are not system line delimiters, I can investigate. All of them should be the system ones.

HeikoKlare commented 3 years ago

That's true. There are some \n line break even though running on a Windows system. I've not yet figured out what produces them, but it's obviously not our code.

Expected: is "\r\nExpected: a ValueBased deeply equal to <ValueBased#1(\r\n        value=\"test\"\r\n        children=[ValueBased#2(value=\"test\")]\r\n)>\r\n     but: found the following differences:\r\n        •  (ValueBased#1).children contained the unexpected value: ValueBased#3(value=\"different\")\r\n        •  (ValueBased#1).children was missing the value: ValueBased#2(value=\"test\")\r\n    for object <ValueBased#1(\r\n        value=\"test\"\r\n        children=[ValueBased#3(value=\"different\")]\r\n)>"
     but: was "\nExpected: a ValueBased deeply equal to <ValueBased#1(\r\n        value=\"test\"\r\n        children=[ValueBased#2(value=\"test\")]\r\n)>\n     but: found the following differences:\r\n        •  (ValueBased#1).children contained the unexpected value: ValueBased#3(value=\"different\")\r\n        •  (ValueBased#1).children was missing the value: ValueBased#2(value=\"test\")\r\n    for object <ValueBased#1(\r\n        value=\"test\"\r\n        children=[ValueBased#3(value=\"different\")]\r\n)>"
jGleitz commented 3 years ago

Hmpf, that’s Hamcrest’s fault:

           description.appendText(reason)
                      .appendText("\nExpected: ")
                      .appendDescriptionOf(matcher)
                      .appendText("\n     but: ");

(in MatcherAssert#assertThat)

jGleitz commented 3 years ago

I pushed a version that hardcodes \n for Hamcrest’s newlines. I like how the test shows how the whole message will look like, this is why I prefer this solution over simply removing newlines.

Do you agree with the solution? Does it work for you?

HeikoKlare commented 3 years ago

It would be absolutely fine for me, but it didn't work out yet to only change the line breaks. I will try to figure out what's going on there, but don't expect me to invest too much time.

In addition, «\n» unfortunately seems to evaluate to System.lineSeparator, so it potentially has to be only \n.

HeikoKlare commented 3 years ago

To be more precise: After fixing the line break issue, the Strings are literally equal but still the matcher evaluates to false.

HeikoKlare commented 3 years ago

I didn't find a way to force the StringConcatenationClient to use two different line delimiters yet. \n is, of course, escaped, thus resolves to \\n. «\n» resolves to System.lineSeparator. Probably there is a way, but I need to move to other topics now.

One solution that works for me is doing the opposite: Replacing the line delimiters in the actual instead of the expected result. exception.message.replaceAll('\r?\\n', System.lineSeparator) would do such a replacement. However, I am not sure whether other systems use even further line delimiters that procude false matches then...

jGleitz commented 3 years ago

I pushed the replaceAll solution. Does it work for you now?

I don’t think it’s worth wasting too much time on, so if it works, I’d leave it this way until somebody finds an issue with it.

HeikoKlare commented 3 years ago

I don’t think it’s worth wasting too much time on

I totally agree.

I pushed the replaceAll solution. Does it work for you now?

Yes, it seems to work. I'll provide a review tomorrow and then we should hopefully be ready to merge.