search

pez-globo / pufferfish-software

All software for the Pufferfish ventilator.
Apache License 2.0
0 stars 1 forks source link

Separate TestCatch2 and TestCoverage build targets #288

Closed renjipanicker closed 3 years ago

renjipanicker commented 3 years ago

Closes #284

Moves all optimization from TestCatch2 to new target TestCoverage

ethanjli commented 3 years ago

Great approach, I hadn't thought of doing it this way, and the structure is clean!

Can you add documentation to firmware/ventilator-controller-stm32/README.md introducing/describing this new TestCoverage target and how to use it correctly? I'm not sure how to get the lcov report, and after running the following command from within firmware/ventilator-controller-stm32 I don't see an html report:

./cmake.sh TestCoverage; cd cmake-build-testcoverage; make -j8; ./TestCoverage; cd ..

Additionally, when I ran this above command I got the following warning from CMake:

CMake Warning at cmake/CodeCoverage.cmake:173 (message):
  Code coverage results with an optimised (non-Debug) build may be misleading
Call Stack (most recent call first):
  CMakeLists.txt:46(include)

I do see that lines 50-51 of CMakeLists.txt are manually setting -O0 as a flag; perhaps we need to add a few lines before line 25 of CMakeLists.txt to set the compile options in a way that CMake understands, something like:

elseif ("${CMAKE_BUILD_TYPE}" STREQUAL "TestCoverage")
    message(STATUS "No optimization, debug info included")
    add_compile_options(-O0 -g)

If we do this, can we remove the set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0"); set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0") lines? And can we use add_compile_options for all the other compile flags currently set via set(CMAKE_CXX_FLAGS, namely -fno-inline -fno-inline-small-functions -fno-default-inline?

I'm running CMake version 3.16.3.

ethanjli commented 3 years ago

The instructions for generating the LCOV report work for me! As discussed in meeting, let's update the CMakeLists file to do some things (setting message, compile options) for the TestCatch2 and TestCoverage build targets in the CMAKE_BUILD_TYPE if-elseif ladder at the top of the file, this will be ready to merge.

Another thing I just thought of: it'd be great if you could update .github/workfllows/firmware.yml to run another job to generate the code coverage report HTML files and upload them as a build artifact; you can refer to https://github.com/pez-globo/pufferfish-software/blob/2f56b711fb95d136e33e101ccfa4f7b4ba0b653b/.github/workflows/firmware.yml#L55 for an example of how I'm already using the actions/upload-artifact action to upload something as a build artifact.

renjipanicker commented 3 years ago

The instructions for generating the LCOV report work for me!

Great!

As discussed in meeting, let's update the CMakeLists file to do some things (setting message, compile options) for the TestCatch2 and TestCoverage build targets in the CMAKE_BUILD_TYPE if-elseif ladder at the top of the file, this will be ready to merge.

This is done, I've just checked in the code.

Another thing I just thought of: it'd be great if you could update .github/workfllows/firmware.yml to run another job to generate the code coverage report HTML files and upload them as a build artifact; you can refer to

https://github.com/pez-globo/pufferfish-software/blob/2f56b711fb95d136e33e101ccfa4f7b4ba0b653b/.github/workflows/firmware.yml#L55 for an example of how I'm already using the actions/upload-artifact action to upload something as a build artifact.

Sounds like a great idea! I'll do it.

rohanpurohit commented 3 years ago
  1. This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
  2. Were any of these contributions also part of work you did for an employer or a client? No.
  3. Does this work include, or is it based on, any third-party work which you did not create? No.
ethanjli commented 3 years ago

@renjipanicker I see there's still an open branch in the repo named "hotfix/cmake-seperate-lcov". Since this PR, with a branch named "hotfix/cmake-separate-lcov" has been merged and that branch has been deleted, is it safe to delete the branch which is still open?

renjipanicker commented 3 years ago

@renjipanicker I see there's still an open branch in the repo named "hotfix/cmake-seperate-lcov". Since this PR, with a branch named "hotfix/cmake-separate-lcov" has been merged and that branch has been deleted, is it safe to delete the branch which is still open?

@ethanjli Yes, please go ahead and delete that branch. Thanks.