Skip to content

Add BlockDevice unittests and fix issues they revealed #12398

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 4 commits into from
Feb 14, 2020
Merged

Add BlockDevice unittests and fix issues they revealed #12398

merged 4 commits into from
Feb 14, 2020

Conversation

michalpasztamobica
Copy link
Contributor

Summary of changes

Add unittests to BlockDevice classes:

  • created BlockDeviceMock in UNITTESTS/stub/BlockDevice_mock.h, using gmock
  • new unit tests added to UNITTESTS/features/storage/blockdevice/ using the aforementioned mock class. Coverage is around 80-90%, the missing coverage are error returns on device failures, etc.
  • BufferBlockDevice moved from moduletests to unittests (now using gmock, instead of HeapBlockDevice). SlicingBlockDevice left in module tests to showcase BD stacking.

Source code fixes (added in a separate commit):

  • ExhaustibleBlockDevice now really returns an error on program() if the memory has not been erased before (according to the documentation).
  • I've chagned MBED_ASSERT usage to run-time error codes. I don't see a reason why incorrect address or size would cause a whole program to crash.
  • MBRBlockDevice::partition had offset and size mixed up. This was never revealed because the existing greentea test split the memory in half (size was equal to offset). We are now unittesting a split into 4 partitions which revealed the mistake.

Impact of changes

Not important.

Migration actions required

None

Documentation

Not required.

Pull request type 

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results 

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

@SeppoTakalo
@VeijoPesonen

@michalpasztamobica
BlockDevice: multiple fixes to BlockDevice classes
10481f2 
Incorrect addresses only cause error return values instead of assertion.
ExhaustibleBlockDevice has working get/set_erase_cycle functions and an
array preventing programming without erase.
Fixed MBRBlockDevice partitioning function.
SeppoTakalo
SeppoTakalo previously approved these changes Feb 10, 2020
View reviewed changes
@mbed-ci
Copy link

mbed-ci commented Feb 10, 2020

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 10, 2020 

09:07:11  [  FAILED  ] 2 tests, listed below:
09:07:11  [  FAILED  ] BufferedBlockModuleTest.unalign_read
09:07:11  [  FAILED  ] BufferedBlockModuleTest.program_and_read_from_device

There are failures in unittests

@michalpasztamobica
unittests: BlockDevice unittests improvements
31ce764 
Moved the existing BufferedBlockDevice to features/storage unittests and switched it to gmock.
Added gmock-based unit tests to all other BlockDevice classes.
SlicingBlockDevice test left as a module test.
@mergify mergify bot dismissed SeppoTakalo’s stale review February 10, 2020 17:38

Pull request has been modified.

@michalpasztamobica
Copy link
Contributor Author

I did not initialize all buffers properly. Once I did I saw the issue and just pushed a fix to it.
Please, restart CI.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 11, 2020

CI restarted

@mbed-ci
Copy link

mbed-ci commented Feb 11, 2020

Test run: FAILED

Summary: 2 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests
  • jenkins-ci/mbed-os-ci_build-IAR

@mergify mergify bot added needs: work and removed needs: CI labels Feb 11, 2020
SeppoTakalo
SeppoTakalo previously approved these changes Feb 12, 2020
View reviewed changes
@mergify mergify bot added needs: CI and removed needs: work labels Feb 12, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 12, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 12, 2020

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@mergify mergify bot dismissed SeppoTakalo’s stale review February 13, 2020 07:00

Pull request has been modified.

@michalpasztamobica
Copy link
Contributor Author

Tests failed because I missed a memory leak in test code. I just pushed a commit fixing it. @SeppoTakalo , please review.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2020

CI restarted

SeppoTakalo
SeppoTakalo previously approved these changes Feb 13, 2020
View reviewed changes
@mbed-ci
Copy link

mbed-ci commented Feb 13, 2020

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 4
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_unittests

@michalpasztamobica
Copy link
Contributor Author

I am sorry, valgrind errors got mixed up with gmock warnings and I haven't noticed them. I will provide a fix for this soon.

@mergify mergify bot dismissed SeppoTakalo’s stale review February 13, 2020 13:20

Pull request has been modified.

@michalpasztamobica
Copy link
Contributor Author

Locally I ran all the newly added tests with valgrind and did not see any errors any more. @0xc0170 , can we verify this with CI?

@mergify mergify bot added needs: CI and removed needs: work labels Feb 13, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Feb 13, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Feb 13, 2020

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 5
Build artifacts

@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 14, 2020
@0xc0170 0xc0170 merged commit bac5ffe into ARMmbed:master Feb 14, 2020
@michalpasztamobica michalpasztamobica deleted the block_device_unittests branch February 14, 2020 08:23
@mergify mergify bot removed the ready for merge label Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

Assignees
No one assigned
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michalpasztamobica @mbed-ci @0xc0170 @SeppoTakalo