Re: [PATCH] [feature-unit-test] General payload tests fixes and additional tests


Guy <guy.tal@...>
 

Hey Damian,

Thank you for your feedback and for the latest patches.
Also your patch in the mail: "[Openmama-dev] [PATCH] UNIT TESTS: Addition of variable NOT_IMPLEMENTED status checking."
from what what I've seen, is extremely useful. It gives ability to have flexible tests for each bridge implementation and for each phase in the development cycle.

Regards,
Guy

On 3/12/2013 1:46 PM, Damian Maguire wrote:
Hey Guy, 

I've added your changes to the feature-unit-test branch, with some minor formatting changes and excluding the disabling of GetSendSubjectValid test. Commit ID: 45c945ed1baefb0b6cc1787391da6dff25198b25

Cheers, 

Damian

Damian Maguire – Senior R&D and OpenMAMA Specialist
IntercontinentalExchange | NYSE Technologies
24-26 Adelaide Exchange | Belfast, BT2 8GD
Tel: +44 2890 822 282 (ext: 452161) | Mob: +44 7540 204 077

From: Guy <guy.tal@...>
Date: Tuesday, November 5, 2013 4:15 PM
To: "openmama-dev@..." <openmama-dev@...>
Subject: [Openmama-dev] [PATCH] [feature-unit-test] General payload tests fixes and additional tests

Hi,

We've now run a complete set of the payload unit tests on the RMDS. This mail notes some issues in the general payload tests, payloadgeneraltests.cpp, and suggest some improved tests for which a patch is offered.

There is no direct support for Windows VS solution/project for the payload tests. We tested the the unit-test in our modified environment. We can't test directly on a cloned OpenMAMA repository without modification, but we supply all the information needed to apply the changes.

Fixed tests

PayloadGeneralTests.IterDestroyValid  

 where:
    result = aBridge->msgPayloadIterDestroy(testIter); //testIter == NULL
    EXPECT_EQ (result, MAMA_STATUS_OK);

why:
    Test does not check valid iterator.  An additional test is proposed in the patch.   

PayloadGeneralTests.IterDestroyInValidIter

where:
    result = aBridge->msgPayloadIterDestroy(NULL);
    EXPECT_EQ (result, MAMA_STATUS_OK);

why:
    testIter is NULL, so the expected result is MAMA_STATUS_NULL_ARG. The qpid implementation returns that too.
   
qpidmsgPayloadIter_destroy (msgPayloadIter iter)
{
    qpidIterImpl* impl = (qpidIterImpl*)iter;

    if (NULL == iter)
    {
        return MAMA_STATUS_NULL_ARG;
    }

solution:
    should change to
    EXPECT_EQ (result, MAMA_STATUS_NULL_ARG);
   

PayloadGeneralTests.IterNextInValidIter 

where:
    output = aBridge->msgPayloadIterNext(NULL, testField, testPayload);
    if (output == NULL)
        FAIL();
why:
    The test incorrectly considers NULL output as failure but passing a null iterator would expect a null output
    see qpid implementation:
    msgFieldPayload
    qpidmsgPayloadIter_next (msgPayloadIter          iter,
                             msgFieldPayload         field,
                             msgPayload              msg)
    {
        qpidIterImpl*         impl      = (qpidIterImpl*) iter;
        qpidmsgPayloadImpl*   msgImpl   = (qpidmsgPayloadImpl*) msg;

        /* Field will be NULL on first call so don't check for it */
        if (NULL == iter || NULL == msg)
        {
            return NULL;
        }

solution:
    change to:
    if (output != NULL)   
   

PayloadGeneralTests.IterBeginInValidIter

where:
    output = aBridge->msgPayloadIterBegin(NULL, testField, testPayload);
    if (output == NULL)
        FAIL();
why:
    The test incorrectly considers NULL output as failure but passing a null iterator would expect a null output
    see also qpid implementation:
    qpidmsgPayloadIter_begin (msgPayloadIter          iter,
                              msgFieldPayload         field,
                              msgPayload              msg)
    {
        qpidIterImpl*         impl      = (qpidIterImpl*)iter;
        qpidmsgPayloadImpl*   msgImpl   = (qpidmsgPayloadImpl*)msg;

        if (NULL == iter || NULL == msg)
        {
            return NULL;
        }

solution:
    change to
    if (output != NULL)
   

PayloadGeneralTests.IterHasNextInValidPayload

where:
    output = aBridge->msgPayloadIterHasNext(testIter, NULL);
    EXPECT_EQ (output, MAMA_STATUS_NULL_ARG);
why:
    msgPayloadIterHasNext() returns boolean the test is wrong and expects a status result MAMA_STATUS_NULL_ARG
solution:
    change:
    EXPECT_EQ (output, 0);
   

PayloadGeneralTests.IterAssociateValid

where:
    result = aBridge->msgPayloadIterAssociate(&testIter, testPayload); //#1
    EXPECT_EQ (result, MAMA_STATUS_OK);

    result = aBridge->msgPayloadIterCreate(&testIter, testPayload);
    EXPECT_EQ (result, MAMA_STATUS_OK);

    result = aBridge->msgPayloadIterAssociate(&testIter, testPayload); //#2
    EXPECT_EQ (result, MAMA_STATUS_OK);

why:
    #1
        1. Because testIter is passed to the function as an address (see &). It should be passed by value.
        2. Also testIter is associated before being created
   

#2
        1. because testIter is passed to the function as an address (see &). It should be passed by value.

solution:
    The Create() call is now the first call. Ampersands are stripped from testIter in the call to msgPayloadIterAssociate(...)
    I provide an additional test "IterAssociateTwiceValid" which checks if association works for 2 payloads reusing one iterator.
   

PayloadGeneralTests.GetSendSubjectInValidMsg

why:
    The test considers the function as not-implemented, but actually should test if message passed as NULL.

where
    TEST_F(PayloadGeneralTests, GetSendSubjectInValidMsg)
    {
        const char**              mySubject (NULL);

        result = aBridge->msgPayloadGetSendSubject(NULL, mySubject);
        EXPECT_EQ (result, MAMA_STATUS_NOT_IMPLEMENTED);
    }

solution:
    change to:
    EXPECT_EQ (result, MAMA_STATUS_NULL_ARG);   
   

PayloadGeneralTests.IterHasNextValid

where:
    output = aBridge->msgPayloadIterHasNext(&testIter, testPayload);
    EXPECT_EQ (output, true);
why:
    testIter parameter is passed as an address (with &). Shouldn't pass by reference.

solution:
    change to
    output = aBridge->msgPayloadIterHasNext(testIter, testPayload);
   

PayloadGeneralTests.IterHasNextInValidPayload   

where:
    output = aBridge->msgPayloadIterHasNext(&testIter, NULL);
    EXPECT_EQ (output, MAMA_STATUS_NULL_ARG);
why:
    testIter parameter is passed as an address (with &). Shouldn't pass by reference.

solution:
    change to
    output = aBridge->msgPayloadIterHasNext(testIter, NULL);


PayloadGeneralTests.CreateFromByteBufferInValidBufferLength

where:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, &testName, testFid, &testBuf, NULL);
    EXPECT_EQ (result, MAMA_STATUS_OK);
why:
    1. argument testBuf is passed as an address (see &) It should be passed by value. even though testBuf is only 'char' (in the test) it is confusing. It should be 'char *'
    2. the last argument should be zero. NULL is just improper even though valid. expected status value should be 'invalid argument'.

solution:
    1.
    change declaration to:
    char                testBuf[256]; //originally was just char!
    2.
    change the call to:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, &testName, testFid, testBuf, 0);   
    3.   
    and then expected value:
    EXPECT_EQ (result, MAMA_STATUS_INVALID_ARG);
   

PayloadGeneralTests.GetFieldAsStringValid

where:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, &testName, testFid, testBuf, testLen);
    EXPECT_EQ (result, MAMA_STATUS_OK);
why:
    1. argument testBuf is passed as an address (see &) should be passed by value. even though testBuf is only 'char' it can is confusing. It should be 'char *'

solution:
    1. change declaraion to:
    char                testBuf[256];   
    2. change to:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, &testName, testFid, testBuf, testLen);   

PayloadGeneralTests.GetFieldAsStringInValidName

where:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, NULL, testFid, testBuf, testLen);
    EXPECT_EQ (result, MAMA_STATUS_OK);
why:
    1. argument testBuf is passed as an address (see &) should be passed by value. even though testBuf is only 'char' it can confuse anyone! it should be 'char *'
solution:
    1. change declaraion to:
    char                testBuf[256];   
    2. change to:
    result = aBridge->msgPayloadGetFieldAsString(testPayload, NULL, testFid, testBuf, testLen);

PayloadGeneralTests.GetSendSubjectValid

This is not a useful test (expects unimplemented function). We suggest it should be disabled for the moment.


Proposed new tests:

PayloadGeneralTests.ToStringValidConsistent

what it does:
    tests that check if the string that comes out of 2 identical payloads is consistent.
   

PayloadGeneralTests.IterNextValidConsistent

what it does:
    tests that traverse FIXED amount of times over the payload and expect to get ALL the payloads fields in the end.
   

PayloadGeneralTests.IterAssociateTwiceValid

what it does:
    test that check association (reuse) of 1 iterator with 2 payloads.
 

Regards,
Guy - Tick42

This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of IntercontinentalExchange Group, Inc. (ICE), NYSE Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.

Join Openmama-dev@lists.openmama.org to automatically receive all group messages.