igraph / rigraph

igraph R package
https://r.igraph.org
542 stars 200 forks source link

TODOs in tests #1455

Closed maelle closed 3 weeks ago

maelle commented 4 weeks ago

One comment per test file, I'll ask for help once the list is done.

maelle commented 4 weeks ago

For test-arpack I'd need help or pointers to similar tests in the C core / Python interface

https://github.com/igraph/rigraph/blob/0931fa1698d8b832342a627dae9ad6ef0d66017d/tests/testthat/test-arpack.R#L98

maelle commented 4 weeks ago

I think the one below can be removed because there is a test with subset of attributes

https://github.com/igraph/rigraph/blob/0931fa1698d8b832342a627dae9ad6ef0d66017d/tests/testthat/test-attributes.R#L66

maelle commented 4 weeks ago

I think the one below can be removed because we added a better test for the hits function in #1449 and the other tests for the to-be-deprecated function names will be removed soon enough.

https://github.com/igraph/rigraph/blob/0931fa1698d8b832342a627dae9ad6ef0d66017d/tests/testthat/test-authority.score.R#L65

maelle commented 4 weeks ago

I'll need a test case for

https://github.com/igraph/rigraph/blob/0931fa1698d8b832342a627dae9ad6ef0d66017d/tests/testthat/test-closeness.R#L25

maelle commented 4 weeks ago

I'll need a test case for

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-scan.R#L107-L108

and

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-scan.R#L126-L127

szhorvat commented 4 weeks ago

TODO: Hub and authority scores make little sense for undirected graphs

If the graph is connected, you can verify that HITS and eigenvector centrality return the same result. That would be a good test.

maelle commented 4 weeks ago

For the one below I will simply use the code from the example

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-iterators.R#L10

maelle commented 4 weeks ago

@szhorvat but for hits didn't we already improve the tests that use the hits_scores() function? If they aren't good enough yet could you please open an issue?

maelle commented 4 weeks ago

For the comment below I do not know what else to test and how

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-largest.cliques.R#L5

maelle commented 4 weeks ago

How:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-largest.independent.vertex.sets.R#L14

maelle commented 4 weeks ago

also how:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-minimal.st.separators.R#L7

maelle commented 4 weeks ago

I wonder what the expectation is supposed to be in:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-get.all.shortest.paths.R#L48-L51

maelle commented 4 weeks ago

@szhorvat I am done looking for the TODOs in the test files, and for many of them I'll need some guidance, but there is no hurry.

szhorvat commented 4 weeks ago

For the comment below I do not know what else to test and how

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-largest.cliques.R#L5

I suppose this refers to the fact that the test does not check that they are maximal or maximum. I don't have a quick enough solution for this at the moment. I would leave this as-is for the moment.

szhorvat commented 4 weeks ago

How:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-largest.independent.vertex.sets.R#L14

Same comment as before. We can check that they are all the same size, but I don't think it's even feasible to check that they are largest/maximum. Checking that they are maximal should be feasible, but still some work. I'd leave it for now.

szhorvat commented 4 weeks ago

also how:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-minimal.st.separators.R#L7

You can check that a separator is minimal by:

EDIT: I think we can use is_min_separator() here directly, which is simpler.

szhorvat commented 4 weeks ago

I wonder what the expectation is supposed to be in:

https://github.com/igraph/rigraph/blob/087f84d43afc2b17c32a4eb5ae5d8b7fbcb4ca37/tests/testthat/test-get.all.shortest.paths.R#L48-L51

No clue what this is. I suggest removal.

maelle commented 3 weeks ago

Thank you!

Done in https://github.com/igraph/rigraph/commit/132362fdee722ed8019ef55b29789d81f2c2caad