common-io: Refactoring FLASH test cases #1883

osloapps-max · 2020-04-07T11:21:41Z

Description

This pull request is rather large (sorry for this), the main contributions is the following:

Refactored flash test cases to use TEST_PROTECT sections to guard against cascading failures.
Refactored flash test cases, moving common code for writing, reading, verifying dummy data out of each test case into global privates. This should simplify maintaining the tests as.
Updated AFQP_IotFlashReadInfo to allow block >= sector >= page >= flash (in other words, moved from "greater than" to "greater than or equal") as this is a valid scenario.

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

vjjunnut · 2020-04-09T15:39:16Z

libraries/abstractions/common_io/test/test_iot_flash.c


-    /* Open flash to initialize hardware. */
-    if( gIotFlashHandle == NULL )


This was added to make sure we can run the tests even if the handle was open by someone else. What was the reason to remove this?

@vjjunnut, as most test also tear-down the handle on finish, this would only really apply to single test cases being run. Is this a common use-case in the test system and what is the gain from it?

My understanding of the way the original tests was structured, running any group tests would result in the first test using the globally opened handle to then close it again and force the following tests to open it for them to succeed.

Note that the caller can still control which handle is being opened as the refactoring also added a global "ltestIotFlashInstance" which can be set prior to running the tests.

That is correct that the first test uses the global handle and then closes it so that the rest of the tests can open it again and close.
Usually there will be only one flash, so the instance will be the same. But even if there are multiple flash parts - we still need to be able to use it, even if it was open by the application which runs the tests. You can argue that application shall not open if its only to run the tests. But on some boards, flash is read first to get some context in startup code.

That is correct that the first test uses the global handle and then closes it so that the rest of the tests can open it again and close.

Usually there will be only one flash, so the instance will be the same. But even if there are multiple flash parts - we still need to be able to use it, even if it was open by the application which runs the tests. You can argue that application shall not open if its only to run the tests. But on some boards, flash is read first to get some context in startup code.

I fully understand that there might be a need to use the flash prior to running the tests. It just seems to me that it makes more sense to ensure that such tests setups leaves the hardware in a known state before running the tests. In other words, if the flash is used prior to the tests, the original owner using the flash should also release it.

Bringing in an external from outside the scope just to close it not only leaves the original owner being uncertain of the state of the handle, what happens if the one using the flash prior to the tests do not actually utilize the handle?

Furthermore, none of the test cases as of now resets the global handle to NULL once closing it, which would let the following tests to believe that it was still valid, even while it was not.

Is there some reason that the original owner cannot release the handle prior to running the tests?

@qiutongs Any thoughts on this?

vjjunnut · 2020-04-09T16:45:31Z

libraries/abstractions/common_io/test/test_iot_flash.c

-    /* Get the flash information. */
-    pxFlashInfo = iot_flash_getinfo( xFlashHandle );
-    TEST_ASSERT_NOT_EQUAL( NULL, pxFlashInfo );
+    if( TEST_PROTECT() )


Thanks for adding this.

vjjunnut · 2020-04-09T16:46:24Z

libraries/abstractions/common_io/test/test_iot_flash.c

-        xFlashHandle = gIotFlashHandle;
-    }
+    /* Open the flash instance */
+    xFlashHandle = iot_flash_open( ltestIotFlashInstance );


We need to bring back checking for the gIotFlashHandle

vjjunnut · 2020-04-09T16:50:13Z

libraries/abstractions/common_io/test/test_iot_flash.c

+            {
+                if( uctestIotFlashReadBuffer[ i ] != ( i & 0xFF ) )
+                {
+                    TEST_ASSERT_MESSAGE( 0, "Data was NOT erased" );


Data was not written.

Thanks for catching this, will look into it and push an update.

vjjunnut · 2020-04-09T16:50:29Z

libraries/abstractions/common_io/test/test_iot_flash.c

+                                           remaining_size );
+            TEST_ASSERT_EQUAL( IOT_FLASH_SUCCESS, lRetVal );
+
+            /* Verify the data is erased. */


Wrong comment

Thanks for catching this, will look into it and push an update.

vjjunnut · 2020-04-09T16:54:23Z

libraries/abstractions/common_io/test/test_iot_flash.c

+
+        if( remaining_size > 0 )
+        {
+            /* Read back the data written */


read back the data erased?

Thanks for catching this, will look into it and push an update.

vjjunnut · 2020-04-09T17:23:11Z

libraries/abstractions/common_io/test/test_iot_flash.c

-                TEST_ASSERT_MESSAGE( 0, "Contents do NOT match when readback" );
-            }
+            /* Write the a full page and verify the data by reading it back */
+            prvIotFlashWriteReadVerify( xFlashHandle, ultestIotFlashStartOffset, pxFlashInfo->ulPageSize );


This function writes and reads the same data. When reading back - that data should be erased data, and not written data. Please modify this call to write and then ReadVerifyErased

Good catch, fixing this asap.

qiutongs · 2020-05-11T22:35:27Z

/bot run checks

qiutongs · 2020-05-12T05:39:46Z

/bot run checks

qiutongs · 2020-05-13T00:07:56Z

There is a build failure that is not related to your PR. But it needs to rebase to the latest master. Sorry for that inconvenience.

Do you mind giving me writing access to your fork? I am help rebase it.
https://github.com/osloapps-max/amazon-freertos

osloapps-max · 2020-05-13T05:23:53Z

There is a build failure that is not related to your PR. But it needs to rebase to the latest master. Sorry for that inconvenience.

Do you mind giving me writing access to your fork? I am help rebase it.
https://github.com/osloapps-max/amazon-freertos

You should have an invite pending now.

qiutongs · 2020-05-13T19:39:09Z

There is a build failure that is not related to your PR. But it needs to rebase to the latest master. Sorry for that inconvenience.
Do you mind giving me writing access to your fork? I am help rebase it.
https://github.com/osloapps-max/amazon-freertos

You should have an invite pending now.

Thank you! I accepted it.

qiutongs · 2020-05-13T19:55:08Z

/bot run checks

qiutongs · 2020-05-13T20:31:19Z

/bot run checks

qiutongs · 2020-05-13T21:46:05Z

/bot run checks

qiutongs · 2020-05-13T21:53:14Z

/bot run checks

Co-authored-by: Max Wennerfeldt <[email protected]> Co-authored-by: qiutongs <songqt01@gmail,com>

osloapps-max mentioned this pull request Apr 7, 2020

common-io test cases will have cascading failures as resources are not freed up #1859

Closed

osloapps-max force-pushed the fix/common-io-flash-test-refactoring branch from 794d3a7 to b2b42a2 Compare April 8, 2020 06:41

vjjunnut reviewed Apr 9, 2020

View reviewed changes

dcgaws requested a review from cobusve April 13, 2020 20:25

qiutongs previously approved these changes May 11, 2020

View reviewed changes

cobusve previously approved these changes May 11, 2020

View reviewed changes

Max Wennerfeldt added 3 commits May 13, 2020 12:47

Updates to tests to use TEST_PROTECT logic

c7d6b8c

Ran uncrustify on test

0705f23

Fixes based on review feedback

cb8b1d6

qiutongs dismissed stale reviews from cobusve and themself via cb8b1d6 May 13, 2020 19:53

qiutongs force-pushed the fix/common-io-flash-test-refactoring branch from a18ef56 to cb8b1d6 Compare May 13, 2020 19:53

qiutongs previously approved these changes May 13, 2020

View reviewed changes

format atca_cert_chain.h

c770ca3

qiutongs dismissed their stale review via c770ca3 May 13, 2020 20:59

qiutongs approved these changes May 14, 2020

View reviewed changes

cobusve approved these changes May 14, 2020

View reviewed changes

qiutongs merged commit 0858694 into aws:master May 14, 2020

alfred2g pushed a commit to alfred2g/amazon-freertos that referenced this pull request May 21, 2020

common-io: Refactoring FLASH test cases (aws#1883)

2efb160

Co-authored-by: Max Wennerfeldt <[email protected]> Co-authored-by: qiutongs <songqt01@gmail,com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

common-io: Refactoring FLASH test cases #1883

common-io: Refactoring FLASH test cases #1883

osloapps-max commented Apr 7, 2020

vjjunnut Apr 9, 2020

osloapps-max Apr 13, 2020

osloapps-max Apr 14, 2020

vjjunnut Apr 15, 2020

vjjunnut Apr 15, 2020

osloapps-max Apr 16, 2020

osloapps-max May 6, 2020

vjjunnut Apr 9, 2020

vjjunnut Apr 9, 2020

vjjunnut Apr 9, 2020

osloapps-max Apr 13, 2020

vjjunnut Apr 9, 2020

osloapps-max Apr 13, 2020

vjjunnut Apr 9, 2020

osloapps-max Apr 13, 2020

vjjunnut Apr 9, 2020

osloapps-max Apr 14, 2020

qiutongs commented May 11, 2020

qiutongs commented May 12, 2020

qiutongs commented May 13, 2020

osloapps-max commented May 13, 2020 •

edited

Loading

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020


		/* Open flash to initialize hardware. */
		if( gIotFlashHandle == NULL )

common-io: Refactoring FLASH test cases #1883

common-io: Refactoring FLASH test cases #1883

Conversation

osloapps-max commented Apr 7, 2020

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiutongs commented May 11, 2020

qiutongs commented May 12, 2020

qiutongs commented May 13, 2020

osloapps-max commented May 13, 2020 • edited Loading

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

qiutongs commented May 13, 2020

osloapps-max commented May 13, 2020 •

edited

Loading