Skip to content

Improve genenral BlockDevice tests #10110

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 27, 2019
Merged

Improve genenral BlockDevice tests #10110

merged 1 commit into from
Mar 27, 2019

Conversation

theamirocohen
Copy link
Contributor

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

@ciarmcom
Copy link
Member

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

@ciarmcom ciarmcom requested review from korjaa and a team March 14, 2019 18:00
davidsaada
davidsaada suggested changes Mar 15, 2019
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please just add these very minor fixes.

features/storage/TESTS/blockdevice/general_block_device/main.cpp Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed. delete ignores null pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

features/storage/TESTS/blockdevice/general_block_device/main.cpp Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

davidsaada
davidsaada approved these changes Mar 17, 2019
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@NirSonnenschein
Copy link
Contributor

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

@NirSonnenschein
Copy link
Contributor

@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
ba6748f 
Fixing all rand issues, aligning erase addresses, moving prints to debug mode.
@theamirocohen
Copy link
Contributor Author

@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
View reviewed changes
features/storage/TESTS/blockdevice/general_block_device/main.cpp
@@ -47,13 +48,27 @@
#include "FlashIAPBlockDevice.h"
#endif

// Debug available
#ifndef MODE_DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

cmonr commented Mar 26, 2019

CI started

cmonr pushed a commit to cmonr/mbed-os that referenced this pull request Mar 26, 2019
Merge pull request ARMmbed#10110 from theamirocohen/improve_gen_bd_tests
f712c2f 
Improve genenral BlockDevice tests
@mbed-ci
Copy link

mbed-ci 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@davidsaada davidsaada davidsaada approved these changes

@korjaa korjaa Awaiting requested review from korjaa

Assignees
No one assigned
Labels
release-version: 5.12.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@theamirocohen @ciarmcom @NirSonnenschein @cmonr @mbed-ci @davidsaada @0xc0170