Title
#code-review
Stefano Bonicatti

Stefano Bonicatti

12/24/2019, 5:56 PM
@João Godinho The integration test is meant to have the table use its logic, beyond also testing the schema, so if you mean filling the table with dummy data, no. That been said, if there’s a way to generate that data through launching commands, that’s fine, I would just call them in the test. But I think there might be a second issue. I’m not sure what’s missing but the Linux distro we run on the CI is running inside Docker; I would’ve expected to at least see one login from the Azure user into the Docker container, but.. yeah
j

João Godinho

12/26/2019, 1:28 PM
Hi @Stefano Bonicatti, thanks for the feedback. I’ve pulled the container and indeed
wtmp
is empty, it doesn’t seem to trigger events. I’m a little lost in here on how to proceed from here. You were suggesting providing a dummy
wtmp
file to the container?
Stefano Bonicatti

Stefano Bonicatti

12/26/2019, 1:37 PM
Yes, though looking at it better I see that it’s possible to write into the wtmpx file with its APIs, namely pututxline
1:38 PM
In general it would be better if the system itself is not changed, even if it’s a test… (so for instance having the table read a file on another path), but the API used expect to operate on a file which is used by the system
j

João Godinho

12/26/2019, 1:40 PM
so point it to a tmp file and populate with some random data?
Stefano Bonicatti

Stefano Bonicatti

12/26/2019, 1:44 PM
I was saying not to care about pointing it to something else, because it seemed that calling utmpname would change it also for other processes, but i misread
1:45 PM
So, if we can generate the test data it would be better, so we have more choice vs giving a prefilled file
1:47 PM
Which seems possible using utmpxname to point to a custom a file and then use pututxline to write it.
j

João Godinho

12/26/2019, 1:50 PM
That's what I was thinking, modifying via utmpxname and populate it via the api
2:11 PM
I think the only way right now would be to use a global variable, to extern and overwrite it in the test
j

João Godinho

12/26/2019, 2:25 PM
hum… shouldn’t the implementation itself use
_PATH_WTMP
instead of hard-coding it?
Stefano Bonicatti

Stefano Bonicatti

12/26/2019, 2:41 PM
It could be better yes, but you still need to specify it, because the default is
_PATH_UTMP
2:43 PM
So it would point to the currently logged in users, not the past
j

João Godinho

12/26/2019, 10:19 PM
I’ve been tinkering around, and found two options: setting the
_PATH_WTMP
(btw the default for it is already
/var/log/wtmp
) via cmake, which I tried but my experience with cmake is 0, so couldn’t make it work yet. the other option is to create tests like the ones for
shell_history
, the problem is identical and they create some tmp dirs and test the functions directly (not the output of a query)
Stefano Bonicatti

Stefano Bonicatti

12/27/2019, 1:07 PM
Well, that would make it more of a unit test, because it would test the table directly and “only”. But that’s fine; depending on how much you want to work on it, the best would be to refactor a bit the table so that you can test the logic directly instead of just checking the
Row
results
1:07 PM
because when something doesn’t work, you would only get an empty value in the column, which doesn’t necessarily mean that’s wrong
1:08 PM
what I mean is, that if you check the Row, you cannot distinguish between a failure and soemthing that simply contains an empty value
1:08 PM
as for the define, that’s not possible to do, because tests do not recompile that code, the tables are in a library.. so the define is already chosen
1:09 PM
and it’s the system one, you cannot change it for the test only
j

João Godinho

12/27/2019, 1:34 PM
I think it's best to refactor the implementation then, given that the table may indeed be empty.
1:36 PM
I thought the define could be used, since everything is compiled during the same run, and one could use of flag to either modify or not a preprocessor definition
1:38 PM
Anyway, you agree with the refactoring as a good option to test the parsing of the file? Not the content of the table itself
1:40 PM
I can slightly modify the current table test to only test if it has rows, no harm done I guess, as builds outside containers do populate the table
2:19 PM
But that’s the problem
2:20 PM
Either you compile the table code to be used for tests or you compile it for production
2:22 PM
In any case, accessing results (and errors) directly, not through Row makes for a better test i believe, yes. The integration test is still valuable to verify the schema and the sql part, but it doesn’t really verify the table logic
j

João Godinho

12/27/2019, 2:23 PM
correct
2:23 PM
I’ll see if I can refactor the code today
2:24 PM
thanks for all the help 🙏
Stefano Bonicatti

Stefano Bonicatti

12/27/2019, 2:27 PM
Thanks for your effort! 😃