LG-14972 | Fix XML fixture; implement match_xml matcher

[n1zyy]

🎫 Ticket

Link to the relevant ticket:
LG-14972

🛠 Summary of changes

Implements a match_xml matcher for rspec.

This is intended to work around an issue where the XML fixture had to be mis-indented in a weird way to match what was actually being generated, because we were comparing them with string equality.

Curiously, REXML::Documents never match:

[11] pry(main)> x = REXML::Document.new('<xml/>')
=> <UNDEFINED> ... </>
[12] pry(main)> y = x.dup
=> <UNDEFINED> ... </>
[13] pry(main)> x == y
=> false

So, run the actual and expected XML documents through a REXML::Formatter to normalize them. We still have to do a string comparison, but stuff like indentation go away.

"Shouldn't we just fix the XML we generate?"

No, we should switch to their REST API and never have to deal with XML again. But that's a bigger story.

[n1zyy]

There does not seem to be a norm around writing tests for matchers, but here's what happen if I add an extra character to the fixture:

[n1zyy]

I don't love this pattern, but I'm not the REXML maintainer.

[n1zyy]

It is not super significant which formatter we use, but Pretty generates nicely-formatted XML that's actually fairly close to what we have.

compact = true keeps <foo>bar</foo> from becoming:

<foo>
  bar
</foo>

[n1zyy]

The diffable line would otherwise print the diff between document and comparison, which is the original XML before normalization. This caused some pulling out of hair, because it's not what we're actually comparing, so the diff shown was not correlated to the failure!

[matthinz]

Agreed that we don't consistently have specs for matchers, but we do have some. I think it would be good to have a quick spec for this, just to document what we expect the behavior to be:

• i give you two xml docs that differ in whitespace only, and we can still tell whether they match
• (bonus points, not required if it is a whole thing) i give you two xml docs that are the same, but reference namespaces slightly differently, and we can still tell whether they match

[n1zyy]

I am not convinced this is the right place for these, and it boggles my mind that I am the first person on the project to want to reuse a method between multiple tests, but I cannot find any example of putting these somewhere else?

[mitchellhenke]

There are various flavors of spec helpers in spec/support, but I'd consider creating a new rspec matcher for something like this too.

[mitchellhenke]

Some, but not all, are unintuitively included in all tests here.

[n1zyy]

Creating a new matcher to test the test for my new matcher feels like the turtles are starting to stack up a little too much.

I'm going to push an update moving this out of SpecHelper and into a new DiffHelper. Let me know if that seems like a reasonable enough approach.

[n1zyy]

@matthinz Thanks for the (offline) pointer to

identity-idp/spec/support/fake_analytics_spec.rb

Line 6 in 89afb54

describe '#have_logged_event' do

I have implemented a test with a handful of examples. I ended up not doing anything with namespaces, because the matcher as written is still very literal and doesn't give them any special treatment. I.e., declaring an unused namespace will still produce a document with it through the Pretty Formatter, and even just changing the order namespaces are mentioned will also result in a non-match.

I started to add a test case showing this, but I think the existing tests showing that changing capitalization or ordering of elements will result in a non-match is more expressive: there is nothing special about namespaces.