This is the thread to discuss testing igraph’s C core: writing unit tests, improving existing tests, improving test coverage. Generally, any testing topic that’s too small to start its own thread for can go here.
@GroteGnoom It seems that there is a problem with using
assert in unit tests: when compiling in
Release configuration, CMake defines
NDEBUG by default.
assert: no code wrapped in
assert will run.
Eventually, we will move to a better unit testing setup, but not before the 0.9 release.
For now, in any future tests you contribute, can you please use
IGRAPH_ASSERT instead of
assert? It works identically. There is no need to correct the old tests using
assert—I’ll do that soon.
What’s the expected behavior if you provide a zero probability distribution?
For example in igraph_callaway_traits_game you should provide a distribution of types for each vertex. This function checks that the distribution is non-negative. But it doesn’t check that at least one value is positive. So if you provide only zeros, this distribution is non-normalizable, and it seems to me like this shouldn’t be allowed.
This this situation be checked for, and return an IGRAPH_EINVAL?
Good catch! Yes, report an error with
IGRAPH_EINVAL. You can check that the value of
maxcum, computed below, is strictly positive.
I am working on an ERRORF macro, (Add lastcit game unit tests by GroteGnoom · Pull Request #1625 · igraph/igraph · GitHub), and I wanted to test it, but errorf always prints to stderr, and the tests can only check stdout. Can we add .err files to check stderr?
I am working on an ERRORF macro,
Thanks for working on an
ERRORF macro, this will be a great addition! Please also add a
FATALF (recently a
FATALF would have been useful when integrating the new version of Bliss).
igraph did not have this feature before because of uncertain compiler support (especially MSVC). However, our minimum requirement for igraph 0.9, VS2015, does support variadic macros, so we’re good.
Can we add .err files to check stderr?
I would omit this for now to keep things simple. I would not test the text of error messages (as opposed to the error code) in general functions. Thus this feature would be used only for testing the error reporting function itself. That function is sufficiently simple and the risk of breakage is sufficiently low that we can verify it manually. A unit test is not needed.
However, should you want to test it anyway, it is possible by setting a new error handler that prints to stdout instead of stderr. I added such a test for (the much less commonly used)
igraph_fatal recently: tests: fatal error handler · igraph/igraph@01dd666 · GitHub
The main thing I want to test is if it works well on all compilers, using the CI pipeline. For example, more complex variadic macro usage can be buggy: c++ - MSVC++ variadic macro expansion - Stack Overflow.
Of course! That makes so much sense .
Should we have printf macros for things like igraph_integer_t? I’m now putting in %ld, but that’s only correct with certain settings. I see some places where variables are cast to long, but that can cast something from 64 bits to 32 bits. Maybe casting everything to 64 bits helps? Then we could use
PRId64. Or we could use
%lld and cast to
(long long), which seems to be 64 bits pretty much everywhere (64-bit computing - Wikipedia).
Indeed, there should be a format specifier for
igraph_integer_t. We plan to overhaul the integer types in igraph “soon” (still, this will take a few months), and the type will likely become platform-specific (i.e. 64-bit when possible, 32-bit otherwise). At that point, I was planning to add a format specifier for
igraph_integer_t, although we have not discussed this in detail yet.
This work is going to involve many big and disruptive changes, so it will happen after the next release only (due in ~1 month)
igraph_integer_t is an
%d works. In tests, I think it’s better not to rely on the fact the knowledge that it’s an
int but to explicitly cast values to
long or whatever) so the code would be immune to future changes to what
igraph_integer_t is. Example:
igraph_integer_t foo = 5; printf("Foo is %d\n", (int) foo);
In almost all tests (which deal with specific cases), we do not need very large numbers, so casting to
int is good enough. In internal code, or user code that must work with any input (not just specific input, as in tests), of course casting to a narrow type risks losing precision, so it’s not acceptable. This is why eventually a format specifier macro will be needed.
Now I realize that you are talking about
ERRORF, not tests. Perhaps we could add an internal format specifier to go with
igraph_integer_t. I would like to keep it non-public for the moment so that we don’t have to commit to it.
I would add it to
igraph_types.h (even if this header is public) with a big fat warning comment that this is not for public use. Maybe name it
IGRAPH_I_PRId? (Renaming at a later stage should be easy, if we decide to do so.)
Here’s an example of a need for it: Add cited_type_game unit tests by GroteGnoom · Pull Request #1632 · igraph/igraph · GitHub
Standard format specifier macros: Fixed width integer types (since C99) - cppreference.com
I’m totally fine with this. Use
IGRAPH_I_PRId to make it clear that it’s internal. Maybe we should also add an
igraph_real_t for sake of consistency?
As far as
printf is concerned, there is no difference between
float gets auto-converted to
double when passed as a variadic function argument). So the only possible floating point type that
igraph_real_t could be that takes something else than
long double. That’s a weird type which is not commonly used, and there is no sign that people would start using it instead of
double in the foreseeable future (in fact, quite the opposite: it’s going out of fashion).
Thus, I don’t think we need this for the floating point case.
@GroteGnoom I just added a format specifier. Can you please update your PRs to use it?
Additionally, please use the spelling “out-degree” with a dash instead of “out degree”. It’s not a big deal, I’ll fix any instances I find manually before merging.
Awesome! I’ll try it out immediately
@GroteGnoom By what name would you like to be acknowledged? Grote Gnoom, Daniel Noom or something else?
Daniel Noom please
I’m working on modularity_matrix and if you put in zero edges, or your weights sum up to zero, every element of the matrix wil be either NaN or inf. Should I just leave that as it is, check for it and warn users, or check for it and give an Error?
Why is there a IGRAPH_EINVVID but no IGRAPH_EINVEID for edges? Right now if an invalid edge id is encountered sometimes an IGRAPH_EINVVID is emitted, but sometimes an ERROR_EINVAL. Should they maybe all be EINVAL? Or should EINVEID be added?
I don’t know; to be honest, I did not even remember that we’ve had
IGRAPH_EINVVID and I was mostly using
IGRAPH_EINVAL for both cases.
We could add
IGRAPH_EINVEID for sake of completeness, but then we would be breaking the API in lots of places where we have used
IGRAPH_EINVAL before to indicate an invalid edge ID. Sooner or later we should do it, and we are breaking lots of stuff in igraph 0.9 anyway, so I don’t have a problem with it if we do it now. But it’s also okay to postpone it to 0.10 instead.
One thing that is definitely not okay, though, is to use
IGRAPH_EINVVID to indicate a problem with an edge ID. The reason is that
igraph_strerror() converts this error code to
"Invalid vertex id", which is clearly misleading if the problem is with an edge ID and not a vertex ID.
So, the bottom line is:
IGRAPH_EINVVIDfor invalid vertex IDs
IGRAPH_EINVALfor invalid edge IDs
- later on we should probably add
IGRAPH_EINVEIDto indicate invalid edge IDs, and at the same time we should evaluate all the usages of
IGRAPH_EINVALto see if
IGRAPH_EINVEIDwould be more appropriate
There is a calloc here:
Which doesn’t get cleaned up after an error here:
Should the FINALLY stack handle that? I’m very confused about its usage in this file, because I see a lot of FINALLY_CLEANs without anything being destroyed.