[PATCH v5 0/2] MR8928: msxml3/tests: add test for IXMLDOMElement_removeAttributeNode.
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
From: Reinhold Gschweicher <pyro4hell+winehq(a)gmail.com> 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"); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8928
From: Reinhold Gschweicher <pyro4hell+winehq(a)gmail.com> --- 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); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/8928
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
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8928#note_115937
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`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8928#note_115938
incorporated the requested changes and rebased on current `master` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/8928#note_115939
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
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8928#note_115942
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/8928#note_116157
participants (3)
-
Nikolay Sivov (＠nsivov) -
Reinhold Gschweicher
-
Reinhold Gschweicher (＠neroburner)