[riot-notifications] [RIOT-OS/RIOT] [WIP] sys: Add RIOT Registry implementation (#10799)
notifications at github.com
Mon Jan 28 14:13:07 CET 2019
danpetry requested changes on this pull request.
1. The testing philosophy needs some attention IMO. Specifically:
- The application that is currently in the tests/ directory combines several testable elements and also requires changes to the makefile to run each test, which doesn’t make it super easy to use or CI-ready. It could make a good example/ application instead, what do you think? To have this would also be a good “advertisement” for the registry
- Please explicitly and individually write tests for all API calls. That is for the registry, registry conversion module, and SFs. (You could have all tests be unit tests and mocking the interfaces using stubs like the dummy storage facility)
- edge cases and things that could potentially break the API (handing over null pointers etc) aren’t tested
- The SFs aren’t tested extensibly – e.g. how do people implement tests for their SF when they add one? I’d suggest a separate test for each.
2. Please make sure the tests, once you’ve got thorough coverage, compile and run without warnings on all boards that aren’t blacklisted. Use `FEATURES_REQUIRED +=` as appropriate as well as blacklisting boards that don’t build the test applications in Murdock
3. folder structure/placement of stuff
- Suggestion: registry_store stuff could go into registry.c to keep it all together? Or the file into the base folder maybe?
- would all SFs and RHs be implemented in sys/registry? Or would they be in with their module? IMO whatever we do for SFs should be done for RHs to keep it consistent.
4. Should we stipulate naming conventions for SF and RH APIs, or just set a precedent through these initial submissions? (Open question.)
5. Would you consider separating out the storage facility implementations into separate PRs? This would make reviewing the whole thing more manageable.
6. Helpers haven’t been implemented in the configuration manager (shell) in the test application to modify access control settings and enable and disable communication interfaces? The architecture doc says this MUST be done… which is wrong, the RDM or the tests/ application?
7. Please bring the submission up to a final quality (i.e. not WIP) before further review of aspects 2-5
8. Please rebase following the merge of PRs 10796 and 10802
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the notifications