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

Add index to XPath information of XDocument equivalence failure message #1181

Merged
merged 5 commits into from
Dec 6, 2019

Conversation

lg2de
Copy link
Contributor

@lg2de lg2de commented Nov 7, 2019

I think, the XDocument equivalence failure message is not helpful when comparing documents with repeating element names, like HTML documents. By just providing the simple XPath it may be very difficult to identify the element with difference:
/html/body/table/tr/td

This PR is a proposal to improve the failure message by writing index number (according XPath syntax) when a specific element was found at least second time on same level:
/html/body/table[2]/tr[17]/td[5]

Additionally

  • I recommend a new setting for editorconfig according to the current code style
  • I changed a test name according to the test content

Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
@dennisdoomen dennisdoomen changed the title add index to XPath information of XDocument equivalence failure message Add index to XPath information of XDocument equivalence failure message Nov 8, 2019
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.

Great idea 👍
I haven't been using the xml assertions much, but having the index adds value.

Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Xml/XmlReaderValidator.cs Outdated Show resolved Hide resolved
@lg2de
Copy link
Contributor Author

lg2de commented Nov 9, 2019

Do you think that this change of error message is a breaking change?
If users asserting XPath using WithMessage existing tests may fail.

@jnyrup
Copy link
Member

jnyrup commented Nov 9, 2019

While it technically can be a breaking change, as in tests failing after upgrading the package, I don't recall we've considered improvements/changes to failure messages as breaking changes.

@dennisdoomen
Copy link
Member

While it technically can be a breaking change, as in tests failing after upgrading the package, I don't recall we've considered improvements/changes to failure messages as breaking changes.

Why would a test fail? Because somebody observe FA's failure messages in an exception assertion?

@lg2de
Copy link
Contributor Author

lg2de commented Nov 12, 2019

While it technically can be a breaking change, as in tests failing after upgrading the package, I don't recall we've considered improvements/changes to failure messages as breaking changes.

Why would a test fail? Because somebody observe FA's failure messages in an exception assertion?

While creating an example I found that my question is not relevant.
Only when asserting FA failure message the change can cause failing tests. But this is not a normal use case.

@jnyrup
Copy link
Member

jnyrup commented Nov 15, 2019

I came to think of another way to combine the Stack and Dictionary into a single graph that keeps track of the xpath indices.

jnyrup@711a7f9

Some very primitive benchmarks:

  • N is the depth of the xml tree.
  • xml tags are named a0...aN

10 children per node.

This PR

|                     Method | N |         Mean |      Error |     StdDev | Ratio |    Gen 0 | Gen 1 | Gen 2 |  Allocated |
|--------------------------- |-- |-------------:|-----------:|-----------:|------:|---------:|------:|------:|-----------:|
| BeEquivalentTo             | 1 |     5.234 us |  0.1092 us |  0.1214 us |  1.00 |   1.3351 |     - |     - |    4.11 KB |
|                            |   |              |            |            |       |          |       |       |            |
| BeEquivalentTo             | 2 |    25.672 us |  0.2558 us |  0.2393 us |  1.00 |   4.6082 |     - |     - |   14.17 KB |
|                            |   |              |            |            |       |          |       |       |            |
| BeEquivalentTo             | 5 | 3,828.342 us | 30.7972 us | 24.0445 us |  1.00 | 648.4375 |     - |     - | 1995.18 KB |

My branch

|                     Method | N |         Mean |      Error |     StdDev | Ratio |    Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------------------------- |-- |-------------:|-----------:|-----------:|------:|---------:|------:|------:|----------:|
| BeEquivalentTo             | 1 |     2.987 us |  0.0256 us |  0.0227 us |  1.00 |   1.0529 |     - |     - |   3.24 KB |
|                            |   |              |            |            |       |          |       |       |           |
| BeEquivalentTo             | 2 |    12.788 us |  0.2393 us |  0.2122 us |  1.00 |   2.9602 |     - |     - |   9.14 KB |
|                            |   |              |            |            |       |          |       |       |           |
| BeEquivalentTo             | 5 | 1,558.388 us | 16.2188 us | 13.5434 us |  1.00 | 289.0625 |     - |     - | 890.32 KB |

4 children per node.

This PR

|                     Method |  N |            Mean |         Error |        StdDev | Ratio |       Gen 0 | Gen 1 | Gen 2 |     Allocated |
|--------------------------- |--- |----------------:|--------------:|--------------:|------:|------------:|------:|------:|--------------:|
| BeEquivalentTo             |  3 |        71.90 us |      1.375 us |      1.351 us |  1.00 |     11.8408 |     - |     - |      36.59 KB |
|                            |    |                 |               |               |       |             |       |       |               |
| BeEquivalentTo             |  5 |     1,346.27 us |     36.026 us |     30.084 us |  1.00 |    224.6094 |     - |     - |     694.78 KB |
|                            |    |                 |               |               |       |             |       |       |               |
| BeEquivalentTo             | 10 | 1,871,360.79 us | 27,158.727 us | 24,075.504 us |  1.00 | 328000.0000 |     - |     - | 1009116.03 KB |

My branch

|                     Method |  N |          Mean |         Error |       StdDev |        Median | Ratio |       Gen 0 | Gen 1 | Gen 2 |    Allocated |
|--------------------------- |--- |--------------:|--------------:|-------------:|--------------:|------:|------------:|------:|------:|-------------:|
| BeEquivalentTo             |  3 |      34.01 us |      0.678 us |     1.170 us |      33.47 us |  1.00 |      7.0190 |     - |     - |     21.63 KB |
|                            |    |               |               |              |               |       |             |       |       |              |
| BeEquivalentTo             |  5 |     534.87 us |      6.195 us |     5.492 us |     535.04 us |  1.00 |    101.5625 |     - |     - |    312.93 KB |
|                            |    |               |               |              |               |       |             |       |       |              |
| BeEquivalentTo             | 10 | 592,578.85 us | 10,082.794 us | 8,419.590 us | 594,102.50 us |  1.00 | 103000.0000 |     - |     - | 317702.62 KB |

@dennisdoomen
Copy link
Member

@jnyrup do you want to do anything with those performance results? Or can this be merged?

@lg2de
Copy link
Contributor Author

lg2de commented Nov 22, 2019

@jnyrup do you want to do anything with those performance results? Or can this be merged?

Good question.

So far, I was unsure what to do.
I did not have time to analyzed in detail was @jnyrup has changed.
If performance is better, then I'm fine.
Should I merge it into "my" PR?

@jnyrup
Copy link
Member

jnyrup commented Nov 22, 2019

@lg2de You can merge the commit into this PR.
Its commit message (hopefully) describes the idea behind the visit count graph.
In the end it does the same as your algorithm, but more lazily to avoid stack iterations, string allocations and dictionary lookups.

Use a counting graph to keep track of xpath index

Currently we use a stack to keep track of the location and a dictionary to keep track of the xpath index.
That has multiple downsides:

  • We need to keep two separate data structure in sync.
  • We're a lot of string allocations on each iterations when computing path as key for the dictionary.
  • When creating the failure message we're doing lookups in two data structures

Instead we can combine their purposes in a single graph where each node has a Name and a Count, where the latter is updated each time we visit a node with the same path.

On each xml traversal we now only need to find the matching sibling or insert a new child.

Creating the failure message is simply the travel from the current node to the root node in reverse order.

Currently we use a stack to keep track of the location and a dictionary to keep track of the xpath index.
That has multiple downsides:

* We need to keep two separate data structure in sync.
* We're a lot of string allocations on each iterations when computing path as key for the dictionary.
* When creating the failure message we're doing lookups in two data structures

Instead we can combine their purposes in a single graph where each node has a `Name` and a `Count`, where the latter is updated each time we visit a node with the same path.

On each xml traversal we now only need to find the matching sibling or insert a new child.

Creating the failure message is simply the travel from the current node to the root node in reverse order.
@lg2de
Copy link
Contributor Author

lg2de commented Nov 29, 2019

@jnyrup why do you prefer

while (current.parent is object)

over

while (current.parent != null)

?

@dennisdoomen
Copy link
Member

The second one can trigger an != operator. The first one is a new C# construct that makes it immediately clear what you're expecting (providing you know the construct ;-))

@jnyrup
Copy link
Member

jnyrup commented Nov 29, 2019

@lg2de
Copy link
Contributor Author

lg2de commented Nov 29, 2019

Mmh, understood. It feels overengineered to me.

I've applied jnyrup improvement (leaving the mentioned statement) and did some cleanup.
Would you like to merge now? Or do you still missing something?

@jnyrup
Copy link
Member

jnyrup commented Nov 29, 2019

Is that

leaving the mentioned statement

referring to this?

// starting new element, add local name to location stack
// to build XPath info

If so, everything looks good to me 👍

@lg2de
Copy link
Contributor Author

lg2de commented Nov 29, 2019

No, sorry.
I was referring to the null check.
;)

@jnyrup
Copy link
Member

jnyrup commented Dec 5, 2019

@dennisdoomen whenever you're ready ;)

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

This class needs a bit of love. I'll take a look after merging this one.

@dennisdoomen dennisdoomen merged commit fbb9f17 into fluentassertions:master Dec 6, 2019
@lg2de lg2de deleted the XElementDescriptive branch December 7, 2019 11:58
@lg2de
Copy link
Contributor Author

lg2de commented Dec 7, 2019

This class needs a bit of love. I'll take a look after merging this one.

What do you mean?

I just want to note, that I think about to provide new implementation for XDocument comparison.
Recently I worked often with XDocument/XElement and FluentAssertions. I found the output is still sometimes not helpful. I would like to have results like using BeEquivalentTo on complex classes. Hope I'll find time for this soon...

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