The committers are authorized under a signed CLA.

• ✅ João Godinho (92e9bce, 329f52f, f7d1827, 4c8d808, 591e1b6, 06d26a1)

@joaogodinho thanks for this PR, looks good!

Please fix that small issue with commas and remember to sign the CLA.

make format and make format-check mat be handy

make format and make format-check mat be handy

Hi @directionless, the tests failure is related to the containers not populating the wtmp file. I'm working on refactoring the table code so that implementation tests are run on all platforms, but table tests only if the table actually contains something.

I've discussed this with Stefano on the code-review channel on slack. As soon as I have the code, I'll also update the first comment to reflect the choices and what was discussed.

The unit test idea looks good, but it does have an important downside, which is that the integration test will never test the conditions on the CI, because the system file won't have records in it.

Unfortunately since the implementation of a table is with free functions and we cannot change the parameters of the generate functions that sqlite has to call, the only possible alternative I see is to use a global variable in the table code, which stores the path to the file the table has to parse.
This will be accessed via extern in the test code and changed to a test controlled path, where you would use the utmp functions to write on it and then retrieve the values.

In theory we could have the CI use a custom wtmp file, put in the system path, but this might not always work when a user launches tests locally.
So the approach I propose also has the added benefit that we have control on what the file contains and we do not alter the system where the test is run on.

Correct, this won't test the table itself in any CI that's run inside a container.

I've spent a good time trying to find a way for the test to call the wtmp lib directly to some tmp file, but I couldn't implement one that wouldn't had multiple #ifdef due the different OSes, which would in the end limit the tests as well, because in Apple and FreeBSD (if setutxdb is not available) we can't set which wtmp file to parse. If we wanted to be pedantic about the tests, we'd still need to have a test for when we can read from the file, and one for when we can't.

Unfortunately I don't think that running them outside a container would change anything, because the VM it's run on it's destroyed after each pipeline run, so it would most likely not have any record in it.

The MacOS VM does (check here), but it's the only one, since Linux is using containers.

My request was due to the fact that we rely on tests not failing in the CI, beyond manual review, to decide if a PR can be merged, or if there are regressions.
A test that never really run on the CI is a tad less useful.

Though I understand that the current situation is not ideal, we should have another structure for implementing table code; so thanks for trying!

I agree, unfortunately I don't have the resources right now to learn more about C++ and OSQuery structure itself.

Lets keep this structure, but please address my comment below.

I will, I also have a minor fix for the test that I probably discard accidentally. 🙏

ping @Smjert 😃