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

Deprecate assertEqualXMLStructure() #4091

Closed
sebastianbergmann opened this issue Feb 12, 2020 · 12 comments
Closed

Deprecate assertEqualXMLStructure() #4091

sebastianbergmann opened this issue Feb 12, 2020 · 12 comments
Assignees
Labels
feature/assertion Issues related to assertions and expectations type/deprecation Something will be/is deprecated
Milestone

Comments

@sebastianbergmann
Copy link
Owner

I have never understood the use case for this assertion and have never seen it used in the wild.

@sebastianbergmann sebastianbergmann added type/backward-compatibility Something will be/is intentionally broken feature/assertion Issues related to assertions and expectations labels Feb 12, 2020
@sebastianbergmann sebastianbergmann added this to the PHPUnit 9.1 milestone Feb 12, 2020
@sebastianbergmann sebastianbergmann self-assigned this Feb 12, 2020
@sebastianbergmann sebastianbergmann added type/deprecation Something will be/is deprecated and removed type/backward-compatibility Something will be/is intentionally broken labels Feb 12, 2020
@povetina
Copy link

povetina commented Jun 8, 2020

That actually doesn't mean people don't use it.
For example, I maintain a project which among other things exports XML data to remote server.
To check that my code generates correct XML file I use precisely assertEqualXMLStructure() function.
Also, it doesn't strike me as the best practice to remove the function completely without providing a new one.

@tim-bezhashvyly
Copy link

Also using it. Now have to work it around. 😔

@tshafer
Copy link

tshafer commented Sep 2, 2020

I am using it as well.

@theseer
Copy link
Collaborator

theseer commented Oct 29, 2020

I'm not sure whether or not this very method or its implementation are the best way to solve the use case it at least for me did cover.

But the general use case is actually rather simple, given that the definition of "equal" with XML documents isn't exactly clear:

<?xml version="1.0" ?>
<root>
    <node />
</root>

./.

<?xml version="1.0" ?>
<root><node /></root>

Are these equal?

If you parse an XML document into a DOM and compare the resulting object graph, they won't be because of additional DOMText-Elements representing the whitespaces for the indentation and the line breaks in the first XML.

In many (most?) contexts though, the whitespace is actually pretty irrelevant. So how would one assert that two structures are "otherwise" equal?

Maybe the method name doesn't (didn't) really transport that intention, but that's what at least I have been using it for.

@theseer
Copy link
Collaborator

theseer commented Oct 29, 2020

Another example:

<?xml version="1.0" ?>
<root a="true" b="true" />

./.

<?xml version="1.0" ?>
<root b="true" a="true" />

Logically (semantically?), the two XML structures are equal. On a string and DOM-Basis, they are not.

@theseer
Copy link
Collaborator

theseer commented Oct 30, 2020

Talked with @sebastianbergmann, and we decided that I'll re-implement this functionality.

A PR will be provided for it hopefully soon, not sure yet as to when exactly I'll have time to do though.

@theseer
Copy link
Collaborator

theseer commented Nov 1, 2020

The PR mentioned re-implements the main aspect of the original, deprecated assertion assertEqualXMLStructure: The comparison of two XML trees for structural identicality.

Two DOM trees are to be considered identical when, after canonicalization, the following holds true:

  • The same child - parent relationship is maintained
  • The same attributes are set on a node
  • The attribute values are identical
  • The textual content of nodes are identical

Looking at the options of the original, deprecated method, this is stricter. And maybe not actually "structural". The original method for instance ignored namespace defining attributes as well as textual content.

If we want to have a mere "grid" without actually looking at the values / text content, this can of course also be implemented - but should be a different method. As I didn't see a use case for that though, I did not reimplement that aspect yet.

My implementation in the PR basically ignores any whitespace and, due to canonicalization, reorders attributes as needed so the order of attributes is not relevant.

The name assertDOMTreesEqualStructurally is subject to change, only pending a better naming suggestion. It should probably reflect the actual assertion better. It might be kept if the "grid" alike implementation is wanted.

@theseer
Copy link
Collaborator

theseer commented Nov 1, 2020

I spent some time searching how people use this method (https://github.com/search?q=assertEqualXMLStructure&type=Code) and am by now quite convinced that most people, me included, used the method wrong.

I used it for templado's engine tests and the fact attribute values as well as text content are actually ignored seems to be a hardly known fact. For me, it was unexpected and quite some of my tests where actually only working by accident.

So for that reason alone, it's good this method is deprecated. I currently believe we should offer various alternatives to replace it though:

  • A method that does what I implemented in my PR (the current method name though is not fitting)
  • A method to assert that, like the original method, two elements are structurally eqal but find a better, clearly describing name and a more sane implementation
  • A method to assert a DOMDocument adheres to an XSD

Opinions?

@RafaelKr
Copy link

I was just wondering why this method was deprecated and landed here. Will the implementation from @theseer be implemented in a future version?

@theseer
Copy link
Collaborator

theseer commented Jul 14, 2022

Possible, even though I'm not fully convinced yet I like the implementation. One of the reasons why it's still in draft.

We at the very least do have to find a better name, as the current name doesn't properly reflect what it's asserting for.

@RafaelKr
Copy link

RafaelKr commented Jul 14, 2022

I had a look at your implementation and noticed this is actually not what I would need. As you really check for identical documentElements a viable function name could be assertIdenticalDOM or assertIdenticalDOMTree. What do you think?

My current use case is that I deserialize XML from a message queue into a PHP Object and then serialize the object back to XML and the output should have the identical parent-child relationship and the elements should have the same attributes. In my case the values are not important. For example it's possible that the Date formats are different. But the elements and attributes should be the same.

So I think a general function which would be helpful would be a function which can:

  1. Check if only parent-child tree is identical
  2. Check that all attribute keys are the same
  3. Check if all attribute keys and attribute values are the same
  4. Check if element values are the same

By default I think 1 should always be checked, and the others are incremental.

So maybe a bitmask could be used. A rough POC could look like this:

class DomAssert {
    // 0b00000001
    const EQUAL_ATTRIBUTE_KEYS = 1 << 0;

    // 0b00000010
    const EQUAL_ATTRIBUTE_KEYS_AND_VALUES = 1 << 1;

    // 0b00000100
    const EQUAL_VALUES = 1 << 2;

    // 0b00001000
    const IGNORE_VALUE_WHITESPACES = 1 << 3;

    public static function assertEqualDOMStructure(\DOMElement $expectedElement, \DOMElement $actualElement, int $comparisonLevel) {
        $expectedDocument = new DOMDocument();
        $expectedDocument->appendChild($expectedDocument->importNode($expectedElement, true));
        $expectedElementXmlString = $expectedDocument->C14N(false);

        $actualDocument = new DOMDocument();
        $actualDocument->appendChild($actualDocument->importNode($actualElement, true));
        $actualElementXmlString = $actualDocument->C14N(false);

        if ($comparisonLevel & static::IGNORE_VALUE_WHITESPACES) {
            // not sure if this actually trims the whitespaces in the node values?
            $expectedDocument->preserveWhiteSpace = false;
            $actualDocument->preserveWhiteSpace = false;
        }

        $expectedDocument->loadXML($expectedElementXmlString);
        $actualDocument->loadXML($actualElementXmlString);

        static::assertEqualDOMStructureRecursive(
            $expectedDocument->documentElement,
            $actualDocument->documentElement,
            $comparisonLevel
        );

    }

    public static function assertEqualDOMStructureRecursive(\DOMElement $expectedElement, \DOMElement $actualElement, int $comparisonLevel) {
        // compare current element tagName

        if ($comparisonLevel & static::EQUAL_ATTRIBUTE_KEYS || $comparisonLevel & static::EQUAL_ATTRIBUTE_KEYS_AND_VALUES) {
            // check attribute keys

            if ($comparisonLevel & static::EQUAL_ATTRIBUTE_KEYS_AND_VALUES) {
                // check the attribute value
            }
        }

        if ($comparisonLevel & static::EQUAL_VALUES) {
            // compare element values
        }

//        for (...) {
//            static::assertEqualDOMStructureRecursive($expectedChildElement, $actualChildElement, $comparisonLevel);
//        }
    }
}

$domAssert = new DomAssert();

// expected element
$ee = new DOMElement();
// actual element
$ae = new DOMElement();

DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_ATTRIBUTE_KEYS);
DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_ATTRIBUTE_KEYS_AND_VALUES);
DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_ATTRIBUTE_KEYS | DomAssert::EQUAL_ATTRIBUTE_KEYS_AND_VALUES); // same as previous line
DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_VALUES);
DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_ATTRIBUTE_KEYS_AND_VALUES | DomAssert::EQUAL_VALUES);
DomAssert::assertEqualDOMStructure($ee, $ae,DomAssert::EQUAL_ATTRIBUTE_KEYS | DomAssert::EQUAL_VALUES);

@theseer
Copy link
Collaborator

theseer commented Jul 28, 2022

I think it becomes more and more obvious the decision to deprecate the original method was a good idea ;)

Regarding your PoC: We have various use cases here merged into one solution. I'm not yet sure we'd need or even want that. Let me think about that a bit more and maybe speak to @sebastianbergmann tomorrow ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/deprecation Something will be/is deprecated
Projects
None yet
Development

No branches or pull requests

6 participants