The unit tests thread

Development
1

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.

1 Like
2

@GroteGnoom It seems that there is a problem with using assert in unit tests: when compiling in Release configuration, CMake defines NDEBUG by default. NDEBUG disables 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.

1 Like
3

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?

4

Good catch! Yes, report an error with IGRAPH_EINVAL. You can check that the value of maxcum, computed below, is strictly positive.

1 Like
5

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?

6

I am working on an ERRORF macro,

Thanks for working on an ERRORF macro, this will be a great addition! Please also add a WARNINGF and 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

2 Likes
7

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 :slight_smile:.

8

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).

9

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)

Right now igraph_integer_t is an int, so %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 int (or 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.

1 Like
10

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.

@tamas @vtraag any objections to adding this?

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_PRId or 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

1 Like
11

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_I_PRIf for igraph_real_t for sake of consistency?

12

As far as printf is concerned, there is no difference between float and double (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 %f in printf is 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.

13

@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.

14

Awesome! I’ll try it out immediately :slight_smile:

15

@GroteGnoom By what name would you like to be acknowledged? Grote Gnoom, Daniel Noom or something else?

16

Daniel Noom please :slight_smile:

1 Like
17

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?

18

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?

19

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:

  • use IGRAPH_EINVVID for invalid vertex IDs
  • use IGRAPH_EINVAL for invalid edge IDs
  • later on we should probably add IGRAPH_EINVEID to indicate invalid edge IDs, and at the same time we should evaluate all the usages of IGRAPH_EINVAL to see if IGRAPH_EINVVID or IGRAPH_EINVEID would be more appropriate
1 Like
20

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.