Re: [PATCH v2] msxml3/test: Add XMLParser Tests

[Nikolay Sivov]

On 02.08.2016 11:02, Alistair Leslie-Hughes wrote:
> With Signed-off.
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/msxml3/tests/xmlparser.c | 196 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 188 insertions(+), 8 deletions(-)
>
> diff --git a/dlls/msxml3/tests/xmlparser.c b/dlls/msxml3/tests/xmlparser.c
> index 32514e3..bf896e4 100644
> --- a/dlls/msxml3/tests/xmlparser.c
> +++ b/dlls/msxml3/tests/xmlparser.c
> @@ -26,8 +26,45 @@
>  #include "windows.h"
>  #include "ole2.h"
>  #include "xmlparser.h"
> +#include "rpcproxy.h"
>  #include "wine/test.h"

Why do you need this?
> -
>
> +static HRESULT hresult_NotifyEvent = S_OK;

Please move this global variable to factory instance.
> static HRESULT WINAPI nodefact_QueryInterface(IXMLNodeFactory *iface,
>          REFIID riid, void **ppvObject)
> @@ -54,34 +91,67 @@ static ULONG WINAPI nodefact_Release(IXMLNodeFactory *iface)
>  }
>
> static HRESULT WINAPI nodefact_NotifyEvent(IXMLNodeFactory *iface,
> -
>    IXMLNodeSource *pSource, XML_NODEFACTORY_EVENT iEvt)
> -
>    IXMLNodeSource *source, XML_NODEFACTORY_EVENT event)
>
> {
> - return E_NOTIMPL;
> - switch(event)
> - {
> -
>    case XMLNF_STARTDOCUMENT:
> -
>        CHECK_EXPECT(NotifyStartDocument);
> -
>        trace("NotifyEvent: XMLNF_STARTDOCUMENT\n");
> -
>        break;
> -
>    case XMLNF_ENDPROLOG:
> -
>        CHECK_EXPECT(NotifyEndProlog);
> -
>        trace("NotifyEvent: XMLNF_ENDPROLOG\n");
> -
>        break;
> -
>    case XMLNF_ENDDOCUMENT:
> -
>        CHECK_EXPECT(NotifyEndDocument);
> -
>        trace("NotifyEvent: XMLNF_ENDDOCUMENT\n");
> -
>        break;
> -
>    case XMLNF_DATAAVAILABLE:
> -
>        CHECK_EXPECT(NotifyDataAvailable);
> -
>        trace("NotifyEvent: XMLNF_DATAAVAILABLE\n");
> -
>        break;
> -
>    default:
> -
>        ok(0, "Unsupported NotifyEvent %p %d\n", source, event);
> -
>        return E_FAIL;
> - }
> -
> - return hresult_NotifyEvent;
>
> }

Arguably trace()'s are not really useful in this context, and will
appear on every parsing test.
> static HRESULT WINAPI nodefact_BeginChildren(IXMLNodeFactory *iface,
>          IXMLNodeSource *pSource, XML_NODE_INFO *pNodeInfo)
>  {
> - return E_NOTIMPL;
> - trace("nodefact_BeginChildren %p %p %d\n", pSource, pNodeInfo,
> -
>            pNodeInfo ? pNodeInfo->dwType : 0);
> - return S_OK;
>
> }
>
> static HRESULT WINAPI nodefact_EndChildren(IXMLNodeFactory *iface,
>          IXMLNodeSource *pSource, BOOL fEmpty, XML_NODE_INFO *pNodeInfo)
>  {
> - return E_NOTIMPL;
> - trace("nodefact_EndChildren %p %d %p %d\n", pSource, fEmpty, pNodeInfo,
> -
>            pNodeInfo ? pNodeInfo->dwType : 0);
> - return S_OK;
>
> }
>
> static HRESULT WINAPI nodefact_Error(IXMLNodeFactory *iface,
>          IXMLNodeSource *pSource, HRESULT hrErrorCode, USHORT cNumRecs,
>          XML_NODE_INFO **ppNodeInfo)
>  {
> - return E_NOTIMPL;
> - CHECK_EXPECT(FactoryError);
> - trace("nodefact_Error %p %08x %d %p\n", pSource, hrErrorCode, cNumRecs, ppNodeInfo);
> - return S_OK;
>
> }
>
> -static HRESULT WINAPI nodefact_CreateNode(IXMLNodeFactory *iface, IXMLNodeSource *pSource,
> -
>    PVOID pNodeParent, USHORT cNumRecs, XML_NODE_INFO **ppNodeInfo)
>
> +static HRESULT WINAPI nodefact_CreateNode(IXMLNodeFactory *iface, IXMLNodeSource *source,
> -
>    void *parent, USHORT recs, XML_NODE_INFO **nodeinfo)
>
> {
> - return E_NOTIMPL;
> - trace("nodefact_CreateNode %p %p %d %p\n", source, parent, recs, nodeinfo);
> -
> - *nodeinfo = NULL;
> -
> - return S_OK;
>
> }
>
> static const IXMLNodeFactoryVtbl nodefactoryVtbl =
> @@ -150,6 +220,114 @@ static void create_test(void)
>      IXMLParser_Release(parser);
>  }
>
> +static void parse_documnet(void)
> +{
> - HRESULT hr;
> - IXMLParser *parser;
> - IStream* stream = NULL;
> - HGLOBAL hMem;
> - LPBYTE bytes;
> -
> - hr = CoCreateInstance(&CLSID_XMLParser30, NULL, CLSCTX_INPROC_SERVER, &IID_IXMLParser, (void**)&parser);
> - if (FAILED(hr))
> - {
> -
>    win_skip("IXMLParser is not available (0x%08x)\n", hr);
> -
>    return;
> - }
> -
> - hMem = GlobalAlloc(GMEM_MOVEABLE, strlen(empty_document));
> - bytes = GlobalLock(hMem);
> - memcpy(bytes, empty_document, strlen(empty_document));
> - GlobalUnlock(hMem);
> -
> - CreateStreamOnHGlobal(hMem, TRUE, &stream);
> -
> - hr = IXMLParser_SetFactory(parser, &thenodefactory);
> - ok(hr == S_OK, "Expected S_OK got 0x%08x\n", hr);
> -
> - hr = IXMLParser_SetInput(parser, (IUnknown *)stream);
> - ok(hr == S_OK, "Expected S_OK got 0x%08x\n", hr);
> -
> - hr = IXMLParser_GetParserState(parser);
> - ok(hr == XMLPARSER_IDLE, "got 0x%08x\n", hr);
> -
> - SET_EXPECT(NotifyStartDocument);
> - SET_EXPECT(NotifyEndDocument);
> - SET_EXPECT(NotifyEndProlog);
> - SET_EXPECT(NotifyDataAvailable);
> - hr = IXMLParser_Run(parser, -1);
> - todo_wine ok(hr == S_OK, "Expected S_OK got 0x%08x\n", hr);
> - todo_wine CHECK_CALLED(NotifyStartDocument);
> - todo_wine CHECK_CALLED(NotifyEndDocument);
> - todo_wine CHECK_CALLED(NotifyEndProlog);
> - todo_wine CHECK_CALLED(NotifyDataAvailable);

Main problem is that this does not test actual callback order - actual
ordering is

- start document;
- create node;
- end children;
- end prolog;
- data available;
- end document;

Please use test sequences facility saxreader.c is using - add_call(),
ok_sequence(), etc.
> START_TEST(xmlparser)
>  {
>      HRESULT hr;
> @@ -160,6 +338,8 @@ START_TEST(xmlparser)
>          return;
>  create_test();
> - parse_documnet();
> - parse_documnet_error();

I guess it was meant to be 'document'.
>  CoUninitialize();
>
> }