Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1369 +/- ##
==========================================
- Coverage 54.26% 54.13% -0.13%
==========================================
Files 138 138
Lines 20618 20636 +18
Branches 5189 5199 +10
==========================================
- Hits 11188 11171 -17
- Misses 6519 6553 +34
- Partials 2911 2912 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
robgjansen
left a comment
There was a problem hiding this comment.
Looks good! Minor issues only :)
ci/container_scripts/test.sh
Outdated
| FLAGS+=("-C" "$CONFIG") | ||
|
|
||
| # Exclude tor tests as we test them in a different workflow | ||
| FLAGS+=("-LE" "tor") |
There was a problem hiding this comment.
Optional nit: prefer writing the long version of the flags if they exist (I don't remember what L and E mean).
src/main/routing/topology.c
Outdated
| @@ -353,7 +359,7 @@ static gboolean _topology_loadGraph(Topology* top, const gchar* graphPath) { | |||
|
|
|||
| _topology_lockGraph(top); | |||
| info("reading graphml topology graph at '%s'...", graphPath); | |||
There was a problem hiding this comment.
Remove the "graphml" from the info log. (And maybe search to check if other log messages or comments refer to graphml).
There was a problem hiding this comment.
Updated these and also the temporary filename for the graph in controller.c.
| </graphml> | ||
| gml: | | ||
| graph [ | ||
| prefer_direct_paths 1 |
There was a problem hiding this comment.
Does this need directed 0 like shown in the other GML graphs?
There was a problem hiding this comment.
The default for gml is undirected, so if the graph is undirected, networkx doesn't include the directed key. I'll update the xml to use the edgedefault="directed" attribute in this conversion test so that we can make sure networkx sets this correctly (directed 1).
There was a problem hiding this comment.
If you look at the other GML graphs in this commit, why do those list directed 0?
There was a problem hiding this comment.
I hand-wrote those graphs and included it just to be explicit, but I can remove those if it's more clear.
There was a problem hiding this comment.
Ahh, OK, this all makes sense now! I think you can just ignore this comment.
This changes igraph to read gml rather than graphml, the conversion scripts to write gml, and the test cases to use gml topologies. The conversion script now requires networkx version >= 2.5, so the config-convert test has been moved out of the required tests.
Part of #778.
Since igraph does not support graph attributes, the
prefers_direct_pathattribute is currently ignored. We plan to remove this attribute and replace it with a shadow configuration optionuse_shortest_path.