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

Make test target name unique. #28

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

gberardi-pillar
Copy link
Contributor

@gberardi-pillar gberardi-pillar commented Jan 3, 2024

everest-core cannot build with tests (BUILD_TESTING=ON) due to submodules having generic names that collide when CMake runs.

This change makes the test target for libevse-security unique by naming it after the repo, a pattern that can be followed for all repos.

NOTE: This change does not enable automated builds and tests, as there are other PRs to handle that work.
I will also point out that when I run the tests manually, 17 out of 26 tests fail.

ALSO NOTE: I changed the flag from BUILD_TESTING_EVSE_SECURITY to BUILD_TESTING, since that variable gets used by CTest and is needed to allow the make test target to work.

I also used the Modern CMake pattern of enabling tests only if the project is the main project (not a child of a parent project) or if it is explicitly specified with cmake -Deverest-evse_security_BUILD_TESTING=ON to allow flexibility.

Signed-off-by: Gianfranco Berardi <[email protected]>
@gberardi-pillar
Copy link
Contributor Author

@shankari Here's a minor change to review.

@gberardi-pillar
Copy link
Contributor Author

@valentin-dimov Since you have contributed the most to this repo, would you be able to look at this PR?

CMakeLists.txt Outdated Show resolved Hide resolved
@Dominik-K
Copy link

Copy link

@shankari shankari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the reason for the comment
#28 (comment)
but I have asked that as a question in
EVerest/EVerest#129 (comment)

I don't see any other questions here, and it would be good to make more progress on cleaning up the tests.

I approve assuming that you will address the comment assuming that it is warranted by the outcome of the larger discussion before merge.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Signed-off-by: Gianfranco Berardi <[email protected]>
@andistorm andistorm merged commit 722d7e0 into EVerest:main Feb 6, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants