I2C Test assert not catching real failures

[maclobdell]

The I2C EEPROM tests should fail on K64F for an other issue. Basically, due to this other issue, the K64F isn't able to read from the EEPROM. But, the ci-test-shield I2C EEPROM tests for writing/reading 10 and 100 bytes are showing OK. This is not correct.

Here is the very verbose (--vv) log for one of the tests. It shows that the number of bytes read is 0 and the Read string is null. But the asserts are not catching this.

[1481314762.72][CONN][RXD] >>> Running case #5: 'I2C -  EEProm WR 10  Bytes'...
[1481314762.77][CONN][RXD]
[1481314762.77][CONN][INF] found KV pair in stream: {{__testcase_start;I2C -  EEProm WR 10  Bytes}}, queued...
[1481314762.78][CONN][RXD] ****
[1481314762.82][CONN][RXD] Buffer Len = `10`, String = `DYOQYKFDB`
[1481314762.83][CONN][RXD] ****
[1481314762.83][CONN][RXD]
[1481314762.83][CONN][RXD] ****
[1481314762.86][CONN][RXD]  Test String = `DYOQYKFDB`
[1481314762.87][CONN][RXD] ****
[1481314762.90][CONN][RXD] num_read: 0, num_written: 10
[1481314762.90][CONN][RXD]
[1481314762.91][CONN][RXD] ****
[1481314762.93][CONN][RXD]  Address = `100`
[1481314762.94][CONN][RXD]  Len = `10`
[1481314762.97][CONN][RXD]  Num Bytes Written = `10`
[1481314762.99][CONN][RXD]  Num Bytes Read = `0`
[1481314763.03][CONN][RXD]  Written String = `DYOQYKFDB`
[1481314763.05][CONN][RXD]  Read String = ``
[1481314763.05][CONN][RXD] ****
[1481314763.11][CONN][INF] found KV pair in stream: {{__testcase_finish;I2C -  EEProm WR 10  Bytes;1;0}}, queued...
[1481314763.16][CONN][RXD] >>> 'I2C -  EEProm WR 10  Bytes': 1 passed, 0 failed

+--------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+
| target       | platform_name | test suite    | test case                          | passed | failed | result | elapsed_time (sec) |
+--------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  EEProm WR 10  Bytes         | 1      | 0      | OK     | 0.34               |
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  EEProm WR 100 Bytes         | 1      | 0      | OK     | 0.74               |
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  EEProm WR 2 Bytes           | 0      | 1      | FAIL   | 0.35               |
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  EEProm WR Single Byte       | 0      | 2      | FAIL   | 0.31               |
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  Instantiation of I2C Object | 1      | 0      | OK     | 0.06               |
| K64F-GCC_ARM | K64F          | tests-api-i2c | I2C -  LM75B Temperature Read      | 1      | 0      | OK     | 0.12               |
+--------------+---------------+---------------+------------------------------------+--------+--------+--------+--------------------+

It looks like one of the assert types is distorting the results from the other asserts.

If these two lines are commented out in void flash_WR(), then the tests result in the expected and correct behavior (fail when it actually fails). However, with these lines not-commented out, the tests result in OK.

    TEST_ASSERT_MESSAGE(strcmp((char *)test_string,(char *)read_string) == 0,"String Written != String Read");
    TEST_ASSERT_MESSAGE(strcmp((char *)read_string,(char *)test_string) == 0,"String Written != String Read");
    //TEST_ASSERT_EQUAL_STRING_MESSAGE((char *)test_string,(char *)read_string,"String read does not match the string written");
    //TEST_ASSERT_EQUAL_STRING_MESSAGE((char *)read_string,(char *)test_string,"String read does not match the string written");
    TEST_ASSERT_EQUAL_MESSAGE(num_written,num_read,"Number of bytes written does not match the number of bytes read");
    DEBUG_PRINTF("\r\n****\r\n Address = `%d`\r\n Len = `%d`\r\n Num Bytes Written = `%d` \r\n Num Bytes Read = `%d` \r\n Written String = `%s` \r\n Read String = `%s` \r\n****\r\n",address,size_of_data,num_written,num_read,test_string,read_string);

@bridadan mentioned that he thinks this has something to do with the use of STATUS_CONTINUE in greentea_failure_handler

@adbridge
@BlackstoneEngineering

[bridadan]

@adbridge Didn't we say at one point that STATUS_CONTINUE was deprecated for some reason like this? Do you remember if there was something wrong with the implementation?

[adbridge]

Yes. Unity and Utest don't handle status continue properly. We never got around to finding a good way to deprecate the functionality though. It's intrinsically built into utest. Sent from my Android phone using TouchDown (www.symantec.com)

…

-----Original Message----- From: Brian Daniels [notifications@github.com] Received: Friday, 09 Dec 2016, 20:55 To: ARMmbed/ci-test-shield [ci-test-shield@noreply.github.com] CC: Anna Bridge [Anna.Bridge@arm.com]; Mention [mention@noreply.github.com] Subject: Re: [ARMmbed/ci-test-shield] I2C Test assert not catching real failures (#16) @adbridge<https://github.com/adbridge> Didn't we say at one point that STATUS_CONTINUE was deprecated for some reason like this? Do you remember if there was something wrong with the implementation? - You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#16 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKrGbRnq5ZYuNQBpOfODCYoBEio90bZnks5rGcA7gaJpZM4LJVqv>. IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[BlackstoneEngineering]

@maclobdell If you've fixed this would you mind submitting a PR with the fix, or at least a gist with the code implementation so @0xc0170 can mainline the fix?

Also, nice find!

[0xc0170]

@maclobdell
Based on the above responses, if you change the handler to return STATUS_ABORT, does it affect results?

[bridadan]

You can also just remove the function greentea_failure_handler and all references to it. utest should default to STATUS_ABORT.

[0xc0170]

You can also just remove the function greentea_failure_handler and all references to it. utest should default to STATUS_ABORT.

I think we want to have continue in this case, as the test result should show all results, not just the last one that failed?

@adbridge Is this tracked anywhere as an issue?

I would like to have this fixed in this repository.

[adbridge]

Marcus was against having continue working at all. Basically if a test case fails at any point that test case overall should fail. So there is an element of good design to test cases that needs to be observed. There are Jira tickets to 1) deprecate status continue and then 2) remove it completely. But these are very low priority and the 2nd one is actually quite complicated to do cleanly .
STATUS_ABORT will only abort the failing test case not the overall tests at that point.

[0xc0170]

Thanks @adbridge . I'll send a patch to remove default handler overwrite,

[bridadan]

@0xc0170 You may have to rethink what is a test case. Normally when testing you never say "it's ok if this fails". That's generally a bad test 😄.

I think what I've done in the past is run the whole binary as a single test (greentea only, not utest), then create my own printed summary at the end. Then I return fail if any of the "cases" are a failure. Not really ideal, but then again this a bit of a strange situation.

[maclobdell]

Just throwing in my 2 cents. In the case of general testing, I agree that if something fails within a group of related tests, it makes sense to stop testing that group and move on to avoid wasting time. However, in the case of CI test shield, it is basically a compliance test, and you want to hit everything at fine granularity and report everything (maybe that can be an optional thing).
Also, it is pretty clear that many boards just won't support everything. Some platforms have limited capabilities (e.g PWM frequency limits, PWM feature not available on a connected pin that usually has it on other platforms, some pins not connected due to limited availability). So, reporting failure in the case where a feature is not supported is a bit harsh. I recommend that the assumptions be made fairly granular and if an assumption fails, then the result is "not supported". And if it should work, but doesn't do to some regression, then that is a fail. A nice table with this information for the platform tested would be really nice too. We could even ask that the table be uploaded to platform pages, so users can find out which features are supported and their level of support.