fsfw/fsfw
fsfw/fsfw
10
3
Fork 6
Code Issues 32 Pull Requests 4 Releases 10 Wiki Activity

Tests can now be built as part of FSFW and versioning moved to CMake #500

Merged
mohr merged 33 commits from KSat/fsfw:mueller/integrated-unittests into development 2021-10-18 14:42:53 +02:00
Conversation 6 Commits 33 Files Changed 56 +424 -387
muellerr commented 2021-10-07 13:35:45 +02:00
Owner
Copy Link

The versioning of the FSFW was moved into the CMakeLists.txt file.
The build system then has access to the version of the FSFW and the built version is now displayed when building the static library.

This PR refactores the tests so they are built as part of the FSFW.
This is done by adding Catch2 with the FetchContent directive.
CMake also checks whether Catch2 is already available as a package, either because the user
submoduled it or because it was installed.

The custom configuration folder testcfg was moved from the user folder
to the actual unittest folder.

The tests can be built by setting the CMake FSFW_BUILD_UNITTESTS option
to TRUE/ON. They are built with the static library and dropped inside
the build folders fsfw directory.

Coverage information can be built with HTML format using the cmake-modules package.
It requires a lcov (Linux) or gcovr (Windows) installation.
The README was updated with the instructions on how to build the unittests (with coverage).

This PR makes the https://egit.irs.uni-stuttgart.de/fsfw/fsfw-tests repository obsolete. The FSFW forks can now supply tests for new features more easily by simply adding the tests in FSFW and building them with the new instructions.

The versioning of the FSFW was moved into the `CMakeLists.txt` file. The build system then has access to the version of the FSFW and the built version is now displayed when building the static library. This PR refactores the tests so they are built as part of the FSFW. This is done by adding Catch2 with the `FetchContent` directive. CMake also checks whether Catch2 is already available as a package, either because the user submoduled it or because it was installed. The custom configuration folder testcfg was moved from the user folder to the actual unittest folder. The tests can be built by setting the CMake FSFW_BUILD_UNITTESTS option to TRUE/ON. They are built with the static library and dropped inside the build folders fsfw directory. Coverage information can be built with HTML format using the `cmake-modules` package. It requires a `lcov` (Linux) or `gcovr` (Windows) installation. The README was updated with the instructions on how to build the unittests (with coverage). This PR makes the https://egit.irs.uni-stuttgart.de/fsfw/fsfw-tests repository obsolete. The FSFW forks can now supply tests for new features more easily by simply adding the tests in FSFW and building them with the new instructions.
muellerr added 2 commits 2021-10-07 13:35:46 +02:00 
This PR refactores the tests so they are built as part of the FSFW.
This is done by adding Catch2 with the FetchContent directive.

A future implementation might also use a system installation of Catch2
by first checking whether Catch2 can already be found as a package

The custom configuration folder testcfg was moved from the user folder
to the actual unittest folder.

The tests can be built by setting the CMake FSFW_BUILD_UNITTESTS option
to TRUE/ON. They are built with the static library and dropped inside
the build folders fsfw directory.
muellerr added this to the v3.0.0 milestone 2021-10-07 13:36:03 +02:00
muellerr added 1 commit 2021-10-07 14:20:49 +02:00 
- More information about FSFW build
muellerr changed title from WIP: Tests can now be built as part of FSFW and versioning moved to CMake to Tests can now be built as part of FSFW and versioning moved to CMake 2021-10-07 14:21:51 +02:00
muellerr requested review from gaisser 2021-10-07 14:22:19 +02:00
muellerr added the
feature
 label 2021-10-07 14:22:31 +02:00
mohr requested changes 2021-10-11 13:39:41 +02:00
mohr left a comment
Owner
Copy Link

Tried to run it, with some problems:
/tests/src/fsfw_tests/unit/testcfg/events/subsystemIdRanges.h includes "common/events/commonSubsystemIds.h", /tests/src/fsfw_tests/unit/testcfg/objects/systemObjectList.h includes "common/objects/commonObjectsList.h" and /tests/src/fsfw_tests/unit/testcfg/returnvalues/classIds.h includes "common/returnvalues/commonClassIds.h", all of which I could not find any trace of. Deleting the includes (and in two cases adjusting the starting value of the enums) made the tests compile and run.

Additional problems below...

Tried to run it, with some problems: `/tests/src/fsfw_tests/unit/testcfg/events/subsystemIdRanges.h` includes `"common/events/commonSubsystemIds.h"`, `/tests/src/fsfw_tests/unit/testcfg/objects/systemObjectList.h` includes `"common/objects/commonObjectsList.h"` and `/tests/src/fsfw_tests/unit/testcfg/returnvalues/classIds.h` includes `"common/returnvalues/commonClassIds.h"`, all of which I could not find any trace of. Deleting the includes (and in two cases adjusting the starting value of the enums) made the tests compile and run. Additional problems below...
muellerr added 1 commit 2021-10-11 13:40:12 +02:00
mohr reviewed 2021-10-11 13:40:40 +02:00
CMakeLists.txt Outdated
@ -30,0 +56,4 @@
FetchContent_MakeAvailable(Catch2)
endif()
configure_file(tests/src/fsfw_tests/unit/testcfg/FSFWConfig.h.in tests/FSFWConfig.h)
mohr commented 2021-10-11 13:40:40 +02:00
Owner
Copy Link

Needed to move this into the root folder, so that the fsfw components could find it.

Needed to move this into the root folder, so that the fsfw components could find it.
muellerr commented 2021-10-11 13:54:41 +02:00
Author
Owner
Copy Link

This was not an issue for the KSat integration and tests in the hosted FSFW example. It should be enough to have an own configure file for the actualy FSFW application

This was not an issue for the KSat integration and tests in the hosted FSFW example. It should be enough to have an own configure file for the actualy FSFW application
muellerr commented 2021-10-11 13:59:26 +02:00
Author
Owner
Copy Link

I can now see what can be problematic.. I am not entirely sure how to solve this

I can now see what can be problematic.. I am not entirely sure how to solve this
mohr commented 2021-10-11 14:18:28 +02:00
Owner
Copy Link

I just changed it to configure_file(tests/src/fsfw_tests/unit/testcfg/FSFWConfig.h.in FSFWConfig.h)

I just changed it to `configure_file(tests/src/fsfw_tests/unit/testcfg/FSFWConfig.h.in FSFWConfig.h)`
mohr reviewed 2021-10-11 13:41:36 +02:00
CMakeLists.txt
@ -30,0 +58,4 @@
configure_file(tests/src/fsfw_tests/unit/testcfg/FSFWConfig.h.in tests/FSFWConfig.h)
configure_file(tests/src/fsfw_tests/unit/testcfg/TestsConfig.h.in tests/TestsConfig.h)
configure_file(tests/src/fsfw_tests/unit/testcfg/OBSWConfig.h.in tests/OBSWConfig.h)
mohr commented 2021-10-11 13:41:36 +02:00
Owner
Copy Link

Seems to be not used atm, only instance is in /hal/src/fsfw_hal/linux/uart/UartComIF.cpp, where it is expected in the root folder as well.

Seems to be not used atm, only instance is in `/hal/src/fsfw_hal/linux/uart/UartComIF.cpp`, where it is expected in the root folder as well.
muellerr commented 2021-10-11 13:53:49 +02:00
Author
Owner
Copy Link

These configuration files are private to the tests. I wil look into it to figure out how to solve this..

These configuration files are private to the tests. I wil look into it to figure out how to solve this..
mohr marked this conversation as resolved
mohr reviewed 2021-10-11 13:42:01 +02:00
CMakeLists.txt
@ -29,1 +40,4 @@
add_library(${LIB_FSFW_NAME})
if(FSFW_BUILD_UNITTESTS)
message(STATUS "Building the FSFW unittests in addition to the static library")
mohr commented 2021-10-11 13:42:01 +02:00
Owner
Copy Link

had to set set(FSFW_CONFIG_PATH tests/src/fsfw_tests/unit/testcfg)

had to set `set(FSFW_CONFIG_PATH tests/src/fsfw_tests/unit/testcfg)`
muellerr commented 2021-10-11 13:53:01 +02:00
Author
Owner
Copy Link

That is possible as well. The path was always set to the user configuration path for my examples

That is possible as well. The path was always set to the user configuration path for my examples
mohr marked this conversation as resolved
mohr commented 2021-10-11 13:44:41 +02:00
Owner
Copy Link

There is an older bug in l.225, where FSFW_CONFIG_PATH is checked but not set to the default value if empty, which then breaks the following checks on it.

Also, the encoded default location is defaultcfg/fsfwconfig while the code location is misc/defaultcfg/fsfwconfig

There is an older bug in l.225, where `FSFW_CONFIG_PATH` is checked but not set to the default value if empty, which then breaks the following checks on it. Also, the encoded default location is `defaultcfg/fsfwconfig` while the code location is `misc/defaultcfg/fsfwconfig`
muellerr added 2 commits 2021-10-11 13:48:38 +02:00
muellerr added 1 commit 2021-10-11 13:51:19 +02:00
muellerr added 1 commit 2021-10-11 13:57:15 +02:00
muellerr commented 2021-10-11 13:57:58 +02:00
Author
Owner
Copy Link

Bug in line 225 fixed and includes removed. It might be worth a thought to simply exit here with a fatal error, but I think it is worth the effort to make it still work out of the box.

I am not sure how to solve the issue with the test specific configuration.. It requires adding the tests path inside the build directoryto the include directories for the tests. But the FSFW already specifies the FSFW_CONFIG_PATH as an interface include path, so there will be a conflict. Right now I can't think of a solution which does not make the tests dependant on the user application FSFW configuration in some way..

Bug in line 225 fixed and includes removed. It might be worth a thought to simply exit here with a fatal error, but I think it is worth the effort to make it still work out of the box. I am not sure how to solve the issue with the test specific configuration.. It requires adding the tests path inside the build directoryto the include directories for the tests. But the FSFW already specifies the FSFW_CONFIG_PATH as an interface include path, so there will be a conflict. Right now I can't think of a solution which does not make the tests dependant on the user application FSFW configuration in some way..
mohr commented 2021-10-11 14:20:42 +02:00
Owner
Copy Link

Regarding the configuration files, I must admit I do not fully understand all the implications of the CMake System here.
But I think when building for unit tests, we can generate the config files in the build path root. I do not think that users will build the unit tests and their FSW in the same build folder/configuration, if that is the problem here.

Regarding the configuration files, I must admit I do not fully understand all the implications of the CMake System here. But I think when building for unit tests, we can generate the config files in the build path root. I do not think that users will build the unit tests and their FSW in the same build folder/configuration, if that is the problem here.
mohr reviewed 2021-10-11 14:22:09 +02:00
CMakeLists.txt Outdated
@ -191,0 +315,4 @@
)
string(CONCAT POST_BUILD_COMMENT
"######################################################################\n"
mohr commented 2021-10-11 14:22:09 +02:00
Owner
Copy Link

And a small one, shouldn't this read "built FSFW [...]"

And a small one, shouldn't this read "*built* FSFW [...]"
mohr reviewed 2021-10-11 14:23:15 +02:00
src/fsfw/FSFWVersion.h.in
@ -0,0 +1,11 @@
#ifndef FSFW_VERSION_H_
#define FSFW_VERSION_H_
const char* const FSFW_VERSION_NAME = "ASTP";
mohr commented 2021-10-11 14:23:15 +02:00
Owner
Copy Link

I think we should remove this as well, not specific to the PR but as we are touching the version anyway...

I think we should remove this as well, not specific to the PR but as we are touching the version anyway...
mohr marked this conversation as resolved
mohr reviewed 2021-10-11 14:29:34 +02:00
tests/src/fsfw_tests/unit/testcfg/events/subsystemIdRanges.h Outdated
@ -0,0 +12,4 @@
*/
namespace SUBSYSTEM_ID {
enum: uint8_t {
SUBSYSTEM_ID_START = 0,
mohr commented 2021-10-11 14:29:34 +02:00
Owner
Copy Link

FW_SUBSYSTEM_ID_RANGE iinstead of 0

`FW_SUBSYSTEM_ID_RANGE` iinstead of `0`
gaisser requested changes 2021-10-11 14:46:41 +02:00
tests/src/fsfw_tests/internal/osal/testCmdExecutor.cpp Outdated
@ -0,0 +1 @@
#include "testCmdExecutor.h"
gaisser commented 2021-10-11 14:44:02 +02:00
Owner
Copy Link

This feature is has not been merged yet.

This feature is has not been merged yet.
gaisser marked this conversation as resolved
tests/src/fsfw_tests/internal/osal/testCmdExecutor.h Outdated
@ -0,0 +1,10 @@
#ifndef FSFW_TESTS_SRC_FSFW_TESTS_INTERNAL_OSAL_TESTCMDEXECUTOR_H_
gaisser commented 2021-10-11 14:44:20 +02:00
Owner
Copy Link

See comment in .cpp

See comment in .cpp
gaisser marked this conversation as resolved
tests/src/fsfw_tests/unit/testcfg/tmtc/apid.h Outdated
@ -0,0 +12,4 @@
*/
namespace apid {
static const uint16_t DEFAULT_APID = 0x00;
static const uint16_t SOURCE_OBSW = 0x73;
gaisser commented 2021-10-11 14:42:12 +02:00
Owner
Copy Link

Copy and Paste error

Copy and Paste error
gaisser marked this conversation as resolved
muellerr added 1 commit 2021-10-11 14:56:54 +02:00
muellerr added 1 commit 2021-10-11 15:03:18 +02:00
muellerr added 1 commit 2021-10-11 15:08:01 +02:00
muellerr added 1 commit 2021-10-11 15:09:37 +02:00
muellerr added 1 commit 2021-10-11 15:41:20 +02:00
muellerr added 1 commit 2021-10-11 15:45:50 +02:00
muellerr added 1 commit 2021-10-11 15:51:39 +02:00
muellerr added 1 commit 2021-10-11 15:56:16 +02:00
muellerr added 1 commit 2021-10-11 16:01:25 +02:00
muellerr added 1 commit 2021-10-11 16:02:11 +02:00
muellerr added 1 commit 2021-10-11 16:04:55 +02:00
muellerr added 1 commit 2021-10-11 16:06:28 +02:00
muellerr added 2 commits 2021-10-11 16:14:46 +02:00
muellerr added 1 commit 2021-10-11 16:17:02 +02:00
muellerr added 1 commit 2021-10-11 16:25:56 +02:00
muellerr added 1 commit 2021-10-11 16:32:32 +02:00
muellerr commented 2021-10-11 16:33:20 +02:00
Author
Owner
Copy Link

All changes and review suggestions added.

All changes and review suggestions added.
mohr reviewed 2021-10-11 17:09:08 +02:00
tests/src/fsfw_tests/unit/testcfg/devices/logicalAddresses.h
@ -0,0 +2,4 @@
#define CONFIG_DEVICES_LOGICALADDRESSES_H_
#include <fsfw/devicehandlers/CookieIF.h>
#include "common/devices/commonAddresses.h"
mohr commented 2021-10-11 17:09:08 +02:00
Owner
Copy Link

another leftover common
Compiler does not seem to mind, ist the devices/ folder used?

another leftover `common` Compiler does not seem to mind, ist the `devices/` folder used?
muellerr commented 2021-10-11 17:10:30 +02:00
Author
Owner
Copy Link

I think it is not used anywhere.. probably can be deleted

I think it is not used anywhere.. probably can be deleted
muellerr commented 2021-10-11 17:13:42 +02:00
Author
Owner
Copy Link

I deleted the include, might keep the file for the future

I deleted the include, might keep the file for the future
mohr marked this conversation as resolved
mohr reviewed 2021-10-11 17:09:42 +02:00
tests/src/fsfw_tests/unit/testcfg/events/translateEvents.cpp Outdated
@ -0,0 +2,4 @@
* @brief Auto-generated event translation file. Contains 81 translations.
* @details
* Generated on: 2021-05-18 16:28:16
*/
mohr commented 2021-10-11 17:09:42 +02:00
Owner
Copy Link

As this file does not seem to contain 81 translations, we might want to delete the header

As this file does not seem to contain 81 translations, we might want to delete the header
muellerr commented 2021-10-11 17:13:20 +02:00
Author
Owner
Copy Link

file deleted

file deleted
mohr marked this conversation as resolved
mohr reviewed 2021-10-11 17:10:02 +02:00
tests/src/fsfw_tests/unit/testcfg/objects/translateObjects.cpp Outdated
@ -0,0 +3,4 @@
* @details
* Contains 69 translations.
* Generated on: 2021-05-18 16:37:37
*/
mohr commented 2021-10-11 17:10:02 +02:00
Owner
Copy Link

same as with the event translation

same as with the event translation
muellerr commented 2021-10-11 17:13:11 +02:00
Author
Owner
Copy Link

file deleted

file deleted
mohr marked this conversation as resolved
muellerr added 1 commit 2021-10-11 17:12:58 +02:00
muellerr added 1 commit 2021-10-11 17:14:41 +02:00
mohr reviewed 2021-10-11 17:15:27 +02:00
CMakeLists.txt
@ -97,0 +193,4 @@
)
elseif(UNIX)
set(COVERAGE_EXCLUDES
"/usr/include/*" "/usr/bin/*" "Catch2/*"
mohr commented 2021-10-11 17:15:27 +02:00
Owner
Copy Link

for me, "*/catch2-src/*" was needed to get catch2 out of the reports.

How do you feel about adding "*/fsfw_tests/*" as well, seems to be a bit too meta to coverage the coverage tests.

for me, `"*/catch2-src/*"` was needed to get catch2 out of the reports. How do you feel about adding `"*/fsfw_tests/*"` as well, seems to be a bit too meta to coverage the coverage tests.
muellerr commented 2021-10-11 17:26:29 +02:00
Author
Owner
Copy Link

I actually installed catch2 and did not have that issue, I can add that directive. Adding the tests makes sense as well

I actually installed catch2 and did not have that issue, I can add that directive. Adding the tests makes sense as well
muellerr commented 2021-10-11 17:31:22 +02:00
Author
Owner
Copy Link

Added the excludes

Added the excludes
mohr commented 2021-10-11 17:47:18 +02:00
Owner
Copy Link

missing a leading * as in "*/catch2-src/*", as catch2 is stashed away somewhere by cmake.

missing a leading * as in `"*/catch2-src/*"`, as catch2 is stashed away somewhere by cmake.
muellerr commented 2021-10-11 17:52:35 +02:00
Author
Owner
Copy Link

Done

Done
mohr self-assigned this 2021-10-11 17:15:56 +02:00
mohr commented 2021-10-11 17:17:25 +02:00
Owner
Copy Link

I can also not find any use of tests/user, is that obsolete as well?

I can also not find any use of `tests/user`, is that obsolete as well?
muellerr added 1 commit 2021-10-11 17:23:03 +02:00
muellerr commented 2021-10-11 17:23:35 +02:00
Author
Owner
Copy Link

It is now. I moved the testtemplate and removed the folder

It is now. I moved the testtemplate and removed the folder
muellerr added 1 commit 2021-10-11 17:31:14 +02:00
muellerr added 1 commit 2021-10-11 17:51:29 +02:00
mohr approved these changes 2021-10-11 17:59:07 +02:00
mohr left a comment
Owner
Copy Link

Phew, I think we're good now. As soon as @gaisser signals contentment, we can close this.

Phew, I think we're good now. As soon as @gaisser signals contentment, we can close this.
muellerr added 1 commit 2021-10-15 15:05:26 +02:00
muellerr added 1 commit 2021-10-15 18:32:38 +02:00
muellerr added 1 commit 2021-10-18 14:36:05 +02:00
gaisser approved these changes 2021-10-18 14:36:29 +02:00
mohr merged commit 9d7d22510b into development 2021-10-18 14:42:53 +02:00
muellerr deleted branch mueller/integrated-unittests 2021-10-18 14:43:10 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
mohr
gaisser
Labels
Clear labels   
API Change

Changes the API, so Users need to adapted

  
Breaking API Change

Changes the API in a non backwards compatible way

  
bug

Something is not working

  
build

Issues related to the buildsystem, including CI

  
cosmetics

Small change most users won't notice

  
Documentation

Improvement of code documentation

  
duplicate

This issue or pull request already exists

  
feature

New feature

  
help wanted

Need some help

  
hotfix

Quick or important fix that is merged directly into master, causing a new revision

  
invalid

#worksforme

  
question

More information is needed

  
Refactor

Contains a refactoring of some code

  
Tests

Issues related to testing code

  
wontfix

This won't be fixed

No Label
API Change
Breaking API Change
bug
build
cosmetics
Documentation
duplicate
feature
help wanted
hotfix
invalid
question
Refactor
Tests
wontfix
Milestone
Clear milestone
No items
No Milestone
v3.0.0
Assignees
Clear assignees
No Assignees
mohr
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: fsfw/fsfw#500
No description provided.
Delete Branch "KSat/fsfw:mueller/integrated-unittests"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?