CI: Add building, testing and linting

[couryrr-afs]

Update the GitHub Actions to run the build and test as well as the linting.

@shankari please review.

[shankari]

@couryrr-afs interesting! Don't you need to change the README as well?
https://github.com/EVerest/libocpp#unit-testing

When I read

cmake .. -DBUILD_TESTING=ON, I thought that was the literal command to run (e.g. .. is the directory to run cmake in). And it looks like the flag is actually -DBUILD_TESTING_LIBOCPP=ON \ and not -DBUILD_TESTING=ON.

Maybe we should remove that section of the README and suggest that people instead refer to the CI for instructions on how to run tests. That will ensure that we don't have two sources of truth that we have to keep consistent.

The code looks fine, as soon as this change is done, I am happy to approve from my side.

[couryrr-afs]

@shankari I thought the same thing about the BUILD_TESTING=ON but was not getting the results I expected. It just came up in a conversation on the mailing list, here, with one of the other devs that:

BUILD_TESTING is the cmake flag, which configures whether to include or exclude testing in everest-core.

[Dominik-K]

Hi @couryrr-afs,

thanks for your first contribution. It's good that you're pushing that there also the build and tests are automatically run.

Looks like you've copied most parts of the workflow from the everest-core repo (see here). That would result in maintaining multiple, >90% similar workflows. (Besides the point that I think it's nice practice to give credit to the source. A simple link would be enough, though, IMO.)

So can you help on improving the workflow in everest-core to become a reuseable workflow instead, please? That would be nice. Thanks.

[shankari]

BUILD_TESTING is the cmake flag, which configures whether to include or exclude testing in everest-core.

But you are using BUILD_TESTING_LIBOCPP=ON.

cmake \
    -B build \
    -S "$EXT_MOUNT/source" \
    -DBUILD_TESTING_LIBOCPP=ON \
    -DCMAKE_BUILD_TYPE=Debug \
    -DCMAKE_INSTALL_PREFIX="$WORKSPACE_PATH/dist"

Is that actually required, or is it BUILD_TESTING?

[couryrr-afs]

@shankari

If I am reading this correctly BUILD_TESTING would allow for the tests to run in everest-core as well. To generate the tests all I appear to need is the BUILD_TESTING_LIBOCPP.

Any guidance from the community would be appreciated if I am interpreting this incorrectly.

[couryrr-afs]

@Dominik-K I can take a look at the reusable workflow. We have a few work items that are all very similar to this so it might be useful.

If time is not in my favor would you just want a simple comment with a link to core's implementation?

[Dominik-K]

@shankari

Maybe we should remove that section of the README and suggest that people instead refer to the CI for instructions on how to run tests. That will ensure that we don't have two sources of truth that we have to keep consistent.

Nice idea 👍

Actually, if we transform the README.md to README.rst, i.e. from Markdown to reStructuredText, we could add the shell-scripts as part of the readme again and still have only one single source of truth 😄 Which is even automatically checked to be up to date 👍

We're using reStructuredText in many places already. But this should be done in another PR.

[Dominik-K]

Regarding the BUILD_TESTING resp. BUILD_TESTING_LIBOCPP: There's an inconsistency. Will be fixed in

• cmake: add LIBOCPP_ prefix to library-own options #301

[shankari]

Actually, if we transform the README.md to README.rst, i.e. from Markdown to reStructuredText, we could add the shell-scripts as part of the readme again and still have only one single source of truth 😄 Which is even automatically checked to be up to date 👍

@Dominik-K this sounds like a great idea but I am not 100% sure how it will happen. Maybe you can link to an example of how the shell scripts from the README are currently automatically checked or elaborate further.

One back of the envelope option is to have the README.rst include a file with the script. But that would also have to be RST. So then is the expectation that the Github action would parse through the included file, treat it as a script, and execute it?

[Dominik-K]

@Dominik-K this sounds like a great idea but I am not 100% sure how it will happen. Maybe you can link to an example of how the shell scripts from the README are currently automatically checked or elaborate further.

Sorry, I've forgot that GitHub deactivated the :include:-directive for README.rst. That was my intended way.

One back of the envelope option is to have the README.rst include a file with the script. But that would also have to be RST. So then is the expectation that the Github action would parse through the included file, treat it as a script, and execute it?

Yeah, then the easiest way would be to simply link to the shell-script from the readme. But that requires context-switching while reading.
Another option would be that https://everest.github.io becomes the single source of truth (and the readme of each repo only links there). But please let's move this discussion out into an extra, specific issue.

[couryrr-afs]

@shankari I cannot add reviewers but there was an update to this PR for the CI work. Please re-review.

[Dominik-K]

@couryrr-afs FYI: I just started trying to reuse the workflow from everest-core. See

• CI: try out using reusable workflow #356

[Dominik-K]

The database_test doesn't succeed. Unclear why because outputs are missing (see comment).

Please make sure that all tests run accordingly before asking for review. Incl. the lint/clang-format.

[couryrr-afs]

The database_test doesn't succeed. Unclear why because outputs are missing (see comment).

Please make sure that all tests run accordingly before asking for review. Incl. the lint/clang-format.

Very sorry for this. It was working before my commit but I had to do some merges. This seems odd now the test that is failing.

[2023-12-19 15:11:13.554057] [0x0000ffffb1018cc0] [info] Established connection to Database.
[2023-12-19 15:11:13.554063] [0x0000ffffb1018cc0] [info] Running SQL initialization script.
/workspaces/libocpp/tests/database_tests.cpp:327: Failure
Expected equality of these values:
db_profile.validFrom.value()
Which is: 2023-12-19T15:11:13.571Z
profile.validFrom.value()
Which is: 2023-12-19T15:11:13.571Z
[2023-12-19 15:11:13.573121] [0x0000ffffb1018cc0] [info] Successfully closed database file
[ FAILED ] DatabaseTest.test_insert_and_get_profiles (19 ms)
[ RUN ] DatabaseTest.test_update_profile_same_profile_id

[Dominik-K]

@couryrr-afs I've figured out the issue:

• OCPP date-time types should have only seconds precision #384

So it's actually already a good result that the unit-tests are run again. 👍 I'm wondering why this wasn't noticed before.

However, I suggest to skip this test for now. As it's now tracked to solve it. I wanted to contribute this change, but it seems you disallowed changes by maintainers. Or?

So I want to commit this change in database_tests.cpp:

TEST_F(DatabaseTest, test_insert_and_get_profiles) {
    // TODO enable again on fixing https://github.com/EVerest/libocpp/issues/384
    GTEST_SKIP() << "validFrom/validTo checks are failing. See https://github.com/EVerest/libocpp/issues/384";

I'm surprised that in the test-result summary it's nowhere mentioned that one test is skipped. Or is it just with my setup?

[Dominik-K]

LGTM with comments (in the meaning: I assume you'll address my comments.)

[Dominik-K]

This line is specific for within the docker-container. I think it can be removed and the source-dir set to the current working-dir in the workflow itself.

[couryrr-afs]

Is there a desire to not run the build and test in a docker container? Currently, that was the process. If there needs to be an update to that process could it be a separate issue? I would like to get this PR merged so that tests are being ran in an automated fashion.

[Dominik-K]

Yes, the scripts run in a Docker-container in GitHub (mainly that the CI could be easily debugged locally, as there no SSH-access, etc. to the CI-runners).

But this script should now also serve as the source for the local run (see #297 (comment)).

However, with the reusable workflow all this files (as all others) will be gone in this repo anyway.

[Dominik-K]

The /ext/ is specific within the docker-container. When you split-up this script in two files (build_install.sh and test.sh), you could run the trap inside workflow. That's not needed when running locally. (However, I hope the files here will soon be replaced with the reusable workflow anyway.)

[Dominik-K]

@shankari Please review again. I'll merge it when you approved it.

Note: It will be replaced with the reusable workflow in

• CI: try out using reusable workflow #356
  anyway.

[shankari]

LGTM as a short-term improvement, soon to be superceded by the reusable workflow.
I'm glad this exposed the issue around timestamps, which is apparently not trivial to fix.
Testing FTW!

@Dominik-K you can go ahead and merge this.

[couryrr-afs]

@Dominik-K and @shankari I am pushing one more change this morning. Looks like it was still running on branch pushes.