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

Fix self-closing XML element is not equivalent to an element without content #1174

Merged
merged 5 commits into from
Nov 9, 2019

Conversation

lg2de
Copy link
Contributor

@lg2de lg2de commented Oct 28, 2019

While testing equivalence of XML documents using the XDocument/XElement extension of FA, I found that "empty elements" are identified as different to "self-closing elements", but e.g. <child></child> should be equivalent to <child />, e.g. according to https://www.w3schools.com/xml/xml_elements.asp

My proposed solution does not look great. But I did not have any other idea to "move forward" and "rewind" the XmlReader.

What do you think?

@jnyrup
Copy link
Member

jnyrup commented Oct 28, 2019

Thanks for taking a stab at this.
With all this conditional logic, the number of tests doesn't seem covering.
E.g. I was able to comment out both !otherReader.IsEmptyElement and && !otherReader.HasValue and all tests still passed.

@lg2de
Copy link
Contributor Author

lg2de commented Oct 28, 2019

Thanks for taking a stab at this.
With all this conditional logic, the number of tests doesn't seem covering.
E.g. I was able to comment out both !otherReader.IsEmptyElement and && !otherReader.HasValue and all tests still passed.

I think, the conditions are required (logically).
I'll try to extend tests.

@jnyrup
Copy link
Member

jnyrup commented Nov 6, 2019

I tried to do some manual mutation testing to get a better gut feeling about the logic.
I wanted to verify that no condition can be omitted or negated.
All cases should have test catching the mutation.

The 6 cases marked with "no test" are the ones that didn't fail.

  • 3 of those failed as expected when negating the condition instead of omitting, so I'm not concerned about those.
  • It seems odd to me that the remaining 3 aren't caught by any tests.

By adding a test with both subject and expected being <parent><child></child></parent> we can kill two mutants. (Unless I messed something up during my testing)

this.reader.NodeType != XmlNodeType.EndElement

  • omitting: no test
  • mutate to ==: When_asserting_a_selfclosing_xml_element_is_equivalent_to_a_different_empty_xml_element_it_should_succeed

this.reader.LocalName != currentName

  • omitting:
  • mutate to ==: When_asserting_a_selfclosing_xml_element_is_equivalent_to_a_different_empty_xml_element_it_should_succeed

|| in MoveToEndElement

  • mutate to &&: no test
  • mutate to ^: When_asserting_an_xml_node_is_equivalent_to_different_xml_node_which_contains_extra_elements_it_should_fail_with_descriptive_message

if (subjectReader.IsEmptyElement && !otherReader.IsEmptyElement)

  • mutate to otherReader.IsEmptyElement: When_asserting_a_selfclosing_xml_element_is_equivalent_to_a_different_empty_xml_element_it_should_succeed
  • omitting !otherReader.IsEmptyElement: no test
  • mutate to !subjectReader.IsEmptyElement: When_asserting_a_selfclosing_xml_element_is_equivalent_to_a_different_empty_xml_element_it_should_succeed
  • omitting subjectReader.IsEmptyElement: subject: "<parent><child></child></parent>", expected: "<parent><child></child></parent>"
  • remove entire condition: When_asserting_a_selfclosing_xml_element_is_equivalent_to_a_different_empty_xml_element_it_should_succeed

else if (otherReader.IsEmptyElement && !subjectReader.IsEmptyElement)

  • mutate to subjectReader.IsEmptyElement: When_asserting_an_empty_xml_element_is_equivalent_to_a_different_selfclosing_xml_element_it_should_succeed
  • omitting !subjectReader.IsEmptyElement: no test
  • mutate to !otherReader.IsEmptyElement: When_asserting_an_empty_xml_element_is_equivalent_to_a_different_selfclosing_xml_element_it_should_succeed
  • omitting otherReader.IsEmptyElement: subject: "<parent><child></child></parent>", expected: "<parent><child></child></parent>"
  • remove entire condition: When_asserting_an_empty_xml_element_is_equivalent_to_a_different_selfclosing_xml_element_it_should_succeed

skipOnce = true; in MoveToEndElement()

  • remove: When_asserting_an_xml_node_is_equivalent_to_different_xml_node_which_contains_extra_elements_it_should_fail_with_descriptive_message

reader.MoveToContent(); in MoveToEndElement()

  • remove: no test

skipOnce = false; in Read()

  • remove: no test

return true; in Read()

  • remove: When_asserting_an_xml_node_is_equivalent_to_different_xml_node_which_contains_extra_elements_it_should_fail_with_descriptive_message

@dennisdoomen
Copy link
Member

@jnyrup you don't have Netflix over there? 😂

@lg2de
Copy link
Contributor Author

lg2de commented Nov 7, 2019

@jnyrup, I added two tests and removed a statement which is not really required.
So, I reduced the number of "surviving mutants".

There are left-overs due to skipOnce.
I have no idea so far to generate a user-level test (public API) to validate.
I tried to create tests directly on XmlReaderWrapper, but failed because internal classes cannot be tested currently in FA. :(

So, I kindly ask to take the current version. The wrapper may (!!!) not need the code when used only in XmlReaderValidator (in the current form). But it is required logically and will be required when it is reused in other parts of FA.

Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, great job 👍

@dennisdoomen dennisdoomen changed the title self closing element is not equivalent to an element without content Fix self-closing XML element is not equivalent to an element without content Nov 9, 2019
@dennisdoomen dennisdoomen merged commit 9050e37 into fluentassertions:master Nov 9, 2019
@lg2de lg2de deleted the XElementSelfClosing branch November 11, 2019 21:14
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.

3 participants