[PATCH v5 0/2] MR8928: msxml3/tests: add test for IXMLDOMElement_removeAttributeNode.

[Reinhold Gschweicher (＠neroburner)]

Implement test cases for `IXMLDOMElement_removeAttributeNode()`
function. Cover the successful removal and failure codes for double
removal and `NULL` pointer removal.

The function is currently unimplemented in wine.

This is the recommended first step according to the contribution
documentation (do coverage MR first, then implementation MR).

WineTestBot submission can be found here:
https://testbot.winehq.org/JobDetails.pl?Key=159816

Once this one is merged I'll create the implementation MR.

I tried to match the formatting. If I've missed something please tell me and I'll fix it.

edit: remove mention of failing tests, update WineTestBot link, fixed by rebasing on 3cfbf9e3cd99dd01d3215c6952ae18ce88207d22

--
  v5: msxml3/tests: incorporate review for domdoc tests
      msxml3/tests: add test for IXMLDOMElement_removeAttributeNode.

https://gitlab.winehq.org/wine/wine/-/merge_requests/8928

[PATCH v5 1/2] msxml3/tests: add test for IXMLDOMElement_removeAttributeNode.

[Reinhold Gschweicher]

From: Reinhold Gschweicher

Implement test cases for `IXMLDOMElement_removeAttributeNode()`
function. Cover the successful removal and failure codes for double
removal and `NULL` pointer removal.

The function is currently unimplemented in wine.
---
 dlls/msxml3/tests/domdoc.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c
index 175919b5169..70284d43294 100644
--- a/dlls/msxml3/tests/domdoc.c
+++ b/dlls/msxml3/tests/domdoc.c
@@ -2218,6 +2218,7 @@ static void test_domnode( void )
     IXMLDOMNode *node = NULL, *next = NULL;
     IXMLDOMNodeList *list = NULL;
     IXMLDOMAttribute *attr = NULL;
+    IXMLDOMAttribute *attr_out = NULL;
     DOMNodeType type = NODE_INVALID;
     VARIANT_BOOL b;
     HRESULT hr;
@@ -2324,8 +2325,41 @@ static void test_domnode( void )
             IXMLDOMAttribute_Release(attr);
         }

+        attr = NULL;
+        hr = IXMLDOMElement_getAttributeNode( element, str, &attr );
+        ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+        ok(attr != NULL, "getAttributeNode returned NULL\n");
+        if (attr)
+        {
+            attr_out = NULL;
+            hr = IXMLDOMElement_removeAttributeNode(element, attr, &attr_out );
+todo_wine {
+            ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+            ok(attr_out != NULL, "removeAttributeNode expected to set attr_out, but got NULL pointer\n");
+}
+            if (attr_out)
+            {
+                /* remove the same attribute again returns invalid arg */
+                hr = IXMLDOMElement_removeAttributeNode( element, attr, NULL );
+                ok(hr == E_INVALIDARG, "removeAttributeNode removed an already removed node, unexpected hr %#lx.\n", hr);
+
+                /* readd removed attribute to recover previous state */
+                hr = IXMLDOMElement_setAttributeNode(element, attr_out, NULL);
+                ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+
+                IXMLDOMAttribute_Release(attr_out);
+            }
+            IXMLDOMAttribute_Release(attr);
+        }
+
         SysFreeString( str );

+        attr = NULL;
+        attr_out = (IXMLDOMAttribute*)0xdeadbeef;
+        hr = IXMLDOMElement_removeAttributeNode( element, attr, &attr_out );
+        todo_wine ok(hr == E_INVALIDARG, "removeAttributeNode removed a NULL pointer hr: %#lx.\n", hr);
+        ok(attr_out == (IXMLDOMAttribute*)0xdeadbeef, "removeAttributeNode expected to not touch attr_out in error case, got (%p)\n", attr_out);
+
         hr = IXMLDOMElement_get_attributes( element, &map );
         ok(hr == S_OK, "get_attributes returned wrong code\n");
         ok( map != NULL, "should be attributes\n");

[PATCH v5 2/2] msxml3/tests: incorporate review for domdoc tests

[Reinhold Gschweicher]

From: Reinhold Gschweicher

---
 dlls/msxml3/tests/domdoc.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c
index 70284d43294..dcda4821125 100644
--- a/dlls/msxml3/tests/domdoc.c
+++ b/dlls/msxml3/tests/domdoc.c
@@ -2329,34 +2329,30 @@ static void test_domnode( void )
         hr = IXMLDOMElement_getAttributeNode( element, str, &attr );
         ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
         ok(attr != NULL, "getAttributeNode returned NULL\n");
-        if (attr)
-        {
-            attr_out = NULL;
-            hr = IXMLDOMElement_removeAttributeNode(element, attr, &attr_out );
+        attr_out = NULL;
+        hr = IXMLDOMElement_removeAttributeNode(element, attr, &attr_out );
 todo_wine {
-            ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
-            ok(attr_out != NULL, "removeAttributeNode expected to set attr_out, but got NULL pointer\n");
+        ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+        ok(attr_out != NULL, "removeAttributeNode expected to set attr_out, but got NULL pointer\n");
 }
-            if (attr_out)
-            {
-                /* remove the same attribute again returns invalid arg */
-                hr = IXMLDOMElement_removeAttributeNode( element, attr, NULL );
-                ok(hr == E_INVALIDARG, "removeAttributeNode removed an already removed node, unexpected hr %#lx.\n", hr);
+        if (attr_out)
+        {
+            /* remove the same attribute again returns invalid arg */
+            hr = IXMLDOMElement_removeAttributeNode( element, attr, NULL );
+            ok(hr == E_INVALIDARG, "removeAttributeNode removed an already removed node, unexpected hr %#lx.\n", hr);

-                /* readd removed attribute to recover previous state */
-                hr = IXMLDOMElement_setAttributeNode(element, attr_out, NULL);
-                ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
+            /* readd removed attribute to recover previous state */
+            hr = IXMLDOMElement_setAttributeNode(element, attr_out, NULL);
+            ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);

-                IXMLDOMAttribute_Release(attr_out);
-            }
-            IXMLDOMAttribute_Release(attr);
+            IXMLDOMAttribute_Release(attr_out);
         }
+        IXMLDOMAttribute_Release(attr);

SysFreeString( str );

-        attr = NULL;
         attr_out = (IXMLDOMAttribute*)0xdeadbeef;
-        hr = IXMLDOMElement_removeAttributeNode( element, attr, &attr_out );
+        hr = IXMLDOMElement_removeAttributeNode( element, NULL, &attr_out );
         todo_wine ok(hr == E_INVALIDARG, "removeAttributeNode removed a NULL pointer hr: %#lx.\n", hr);
         ok(attr_out == (IXMLDOMAttribute*)0xdeadbeef, "removeAttributeNode expected to not touch attr_out in error case, got (%p)\n", attr_out);

[Reinhold Gschweicher (＠neroburner)]

On Tue Sep 16 08:52:43 2025 +0000, Nikolay Sivov wrote:
> Please get rid of this nesting, no need to check for nulls for something
> that's not expected to be null. The 'attr_out' check should stay of
> course, until it's implemented in wine.

removed

[Reinhold Gschweicher (＠neroburner)]

On Tue Sep 16 08:52:43 2025 +0000, Nikolay Sivov wrote:
> You don't need 'attr' here, just use NULL literal for argument value.

replaced with `NULL`

[Reinhold Gschweicher (＠neroburner)]

incorporated the requested changes and rebased on current `master`

[Reinhold Gschweicher (＠neroburner)]

On Tue Sep 16 08:58:27 2025 +0000, Reinhold Gschweicher wrote:
> incorporated the requested changes and rebased on current `master`

Current WineTestBot for domdoc tests: https://testbot.winehq.org/JobDetails.pl?Key=159880

[Nikolay Sivov (＠nsivov)]

On Tue Sep 16 08:58:27 2025 +0000, Reinhold Gschweicher wrote:
> Current WineTestBot for domdoc tests: https://testbot.winehq.org/JobDetails.pl?Key=159880

Please squash commits.