Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve genenral BlockDevice tests #10110

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
7 participants
@theamirocohen
Copy link
Contributor

commented Mar 14, 2019

Description

Fixing all rand issues and aligning erase addresses by assigning explicit addresses, and by that removing FlashIAP restrictions.
Moving prints to debug mode.

This PR depends on #10108

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

Reviewers

@ARMmbed/mbed-os-storage
@korjaa

@theamirocohen theamirocohen force-pushed the theamirocohen:improve_gen_bd_tests branch to d4b9c96 Mar 14, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

@theamirocohen, thank you for your changes.
@korjaa @ARMmbed/mbed-os-storage @ARMmbed/mbed-os-maintainers please review.

@ciarmcom ciarmcom requested review from korjaa and ARMmbed/mbed-os-maintainers Mar 14, 2019

@davidsaada
Copy link
Contributor

left a comment

Looks good. Please just add these very minor fixes.

}

end:
delete[] write_block;
delete[] read_block;
if (read_block) {

This comment has been minimized.

Copy link
@davidsaada

davidsaada Mar 15, 2019

Contributor

Not needed. delete ignores null pointers.

This comment has been minimized.

Copy link
@theamirocohen

theamirocohen Mar 17, 2019

Author Contributor

done

}

end:
delete[] write_block;
delete[] read_block;
if (read_block) {

This comment has been minimized.

Copy link
@davidsaada

davidsaada Mar 15, 2019

Contributor

Same here.

This comment has been minimized.

Copy link
@theamirocohen

theamirocohen Mar 17, 2019

Author Contributor

done

@0xc0170 0xc0170 added needs: work and removed needs: review labels Mar 15, 2019

@davidsaada
Copy link
Contributor

left a comment

LGTM.

@theamirocohen theamirocohen force-pushed the theamirocohen:improve_gen_bd_tests branch to 5dd6144 Mar 18, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@korjaa please review or comment if you don't plan to

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

@theamirocohen is this just a test update? there seem to be changes in non-test code like:
components/storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.cpp

Improve general Block Device tests
Fixing all rand issues, aligning erase addresses, moving prints to debug mode.

@theamirocohen theamirocohen force-pushed the theamirocohen:improve_gen_bd_tests branch from 5dd6144 to ba6748f Mar 26, 2019

@theamirocohen

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@theamirocohen is this just a test update? there seem to be changes in non-test code like:
components/storage/blockdevice/COMPONENT_FLASHIAP/FlashIAPBlockDevice.cpp

It was the #10108 PR code.
The code is now rebased.

@cmonr

cmonr approved these changes Mar 26, 2019

@@ -47,13 +48,27 @@
#include "FlashIAPBlockDevice.h"
#endif

// Debug available
#ifndef MODE_DEBUG

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 26, 2019

Contributor

I thought we had other debug macros.
Or is this the pattern used within the storage tests?

This comment has been minimized.

Copy link
@cmonr

cmonr Mar 26, 2019

Contributor

@theamirocohen @davidsaada Would appreciate an answer here (I realise I only asked four hrs ago), but won't hold up the merge on it.

If it need to be fixed, it can be brought in later.

This comment has been minimized.

Copy link
@davidsaada

davidsaada Mar 26, 2019

Contributor

This is unrelated to other debug macros. Original test code poured a lot of unnecessary prints during the test. Current change removed them (among the rest), by simply using the DEBUG_PRINT define, which is turned off by default depending on MODE_DEBUG. Guess these prints could have been removed altogether, but @theamirocohen took precautions by keeping them in for the case someone should want to see them while debugging.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

CI started

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 26, 2019

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 26, 2019

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr merged commit 2520c77 into ARMmbed:master Mar 27, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 10034 cycles
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.