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

newt: Add pkg.ign_pkgs to skip certain packages when linking #493

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frippertronics
Copy link

@frippertronics frippertronics commented Nov 10, 2022

I've been working on adding unit tests with mocking to an existing project using myNewt, but I've not been able to make it work with existing functionality (such as ignore_file and build-time hooks).

Using FFF, I mocked some function calls to flash_map.c in a unit test for a module. When not using this branch, I get duplicate_symbols errors when linking; however, with the changes in this branch the test runs the mocks as expected. The files to be ignored are set in the module's "test/pkg.yml"-file:
pkg.ign_pkgs: "@apache-mynewt-core/sys/flash_map"

This is probably not the best way to do it, but it works. Any feedback is appreciated!

This allows for files to be mocked for unit testing purposes.
@frippertronics
Copy link
Author

I'll add the required documentation once this method of ignoring packages, or an alternative, has been approved.

@andrzej-kaczmarek
Copy link
Contributor

I don't this options makes sense. The packages are included in build via explicit dependencies from other packages, so having an option to allow any package to remove an arbitrary package from build simply breaks the whole concept of dependencies.

I'd rather focus on changing flash_map or its dependers so either it's not included in all builds or have an option to stub it. Perhaps if you can elaborate a bit more on what you did or are trying to achieve, I'd try to propose an alternate solution.

@frippertronics
Copy link
Author

frippertronics commented Nov 17, 2022

Thanks for the response.

What I want to do is be able to isolate modules from their dependencies when unit testing through mocking. I could create stubs specifically for this case, but the behaviour I want from the stub might differ between the tests.

Edit: The stub behaviour could be customized for each test through global variables, so there's a solution for that problem.

Given this case:
My module (say sys_storage.c) uses flash_map.c to store and retrieve data from the flash storage on my target. I want to simulate that sys_storage.c is able to detect flash wear, so I have two unit tests for sys_storage.c that call my sys_storage_load_and_verify_integrity() , which has calls to flash_area_read() to get the data and checksum.

In the unit test I can either massage the flash map in flash_map.c to fit both these test cases, or I can use a mocking framework such as FFF to mock the function calls to flash_area_read() and return the values I want to test with. The advantage of mocking the function calls is that I avoid having to massage the underlying module to do as I want, and I don't have to rely on the module's functionality. The complexity of the test also increases when my module contains other modules that I have to massage.

I could create stubs for flash_map.c that I compile in only for my sys_storage.c-test, but won't I get a duplicate definitions when other modules specify that they need the flash_map.c and not flash_map_stub.c?

@frippertronics
Copy link
Author

Ideally I would use CMock but getting newt to call ruby to generate mocks during compilation seems a bit more difficult than using FFF.

@andrzej-kaczmarek
Copy link
Contributor

andrzej-kaczmarek commented Nov 17, 2022

Just a quick thought:

andk@x1n:~/devel/mynewt$ newt target revdep blinky
Reverse dependency graph (dependee <-- [dependers]):
--- 8< ---
    * @apache-mynewt-core/sys/flash_map <-- [@apache-mynewt-core/hw/bsp/nordic_pca10095 @apache-mynewt-core/sys/sysinit]
--- 8< ---
    * @apache-mynewt-core/sys/sysinit <-- [@apache-mynewt-core/kernel/os]
--- 8< ---

So flash_map is included in every build by sys/sysinit which does not seem to use it at all. I'd try to remove that dependency and see what happens. I suspect this will break something in other builds, but perhaps we can figure this out and add proper dependencies somewhere else. You can then just use dependencies in your BSP to pull proper flash_map or mocked one.

@frippertronics
Copy link
Author

flash_map was just an arbitrarily chosen package to test this out with, this is also the case for other packages. For many cases it would be beneficial to use a combination of mocked and implemented modules. Accounting for all the cases where a module is used or mocked sounds like it would create a very complex net of pkg.yml-files. I think it would be easier to solve this at link-time instead of at compile-time. It is useful that myNewt resolves dependencies as well as it does, but it makes it more difficult to control what is actually linked into the final executable.

I should probably stress that my feature, ign_pkgs, is only run at link-time and should only be used for unit tests.

I don't know if this makes it any clearer what my purpose is.

@andrzej-kaczmarek
Copy link
Contributor

So if this is used for unit tests (i.e. newt test) perhaps a better idea is to introduce mock packages that can be added to unit tests build only and they will replace other package, smth like this:

pkg.type: mock
pkg.mock: @apache-mynewt-core/sys/flash_map

This way you don't break dependencies since package is replaced instead of being ignored. Perhaps mock packages should have also some restrictions, e.g. they can only change implementation and other settings are used from original package, but this is tbd. I'd like to avoid sceanrios where dependencies need to be resolved again or smth since that process is complicated enough already :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants