Test suite relies too heavily on dot_parser, fails to test Python API logic #385

ferdnyc · 2024-07-16T13:33:00Z

#383 demonstrated not only an error in my code, but a deficiency in our current test suite.

Even though there are hundreds of tests run on every pydot commit, the overwhelming majority of them follow the same pattern: Use a .dot file as input, load it in using pydot.graph_from_dot_file(), and then ensure that the graph image produced by pydot.Dot.create() is the same as what's produced by running the original file through dot -Tjpe.

That works well for testing the parser, but it falls short of testing the API logic because it won't exercise any code paths that are only accessible via the Python API, and those code paths are growing in number and complexity as we add automatic quoting logic and other user-friendly enhancements that the parser will never take advantage of.

So, things like the quote_id_if_necessary() logic will never be triggered by graph data generated in the parser. Any values that must be quoted, it will read as already quoted (they have to be, for the input data to be valid), so the quotes will already be present in its input data.

In other words, when dot_parser reads the following:

digraph "graph" {
"Node A";
"Node B";
"Node A" -> "Node B";
}

it effectively makes these API calls:

import pydot
g = pydot.Graph('"graph"', graph_type="digraph")
g.add_node(pydot.Node('"Node A"'))
g.add_node(pydot.Node('"Node B"'))
g.add_edge(pydot.Edge('"Node A"', '"Node B"'))

Now that we've added logic so that these calls are equivalent:

g = pydot.Graph("graph", graph_type="digraph")
g.add_node(pydot.Node("Node A"))
g.add_node(pydot.Node("Node B"))
g.add_edge(pydot.Edge("Node A", "Node B"))

The only way to properly test THOSE calls is to write them in a test function. The parser can't do it for us.

We need a lot more API-driven tests.

(Edit: Now with actually-valid sample code, as I'm no longer writing digraph statements in an undirected graph block.)

The text was updated successfully, but these errors were encountered:

ferdnyc · 2024-07-16T14:01:59Z

This is also why we need to get coverage collection spun up on our test suite, so that we'll KNOW which code isn't being tested despite our massive collection of tests.

I've been poking around the edges of that somewhat, on-and-off, but there's a complication in that I got us switched over to using unittest-parallel as our test runner, and collecting coverage data when tests are run under multiprocessing turns out to be a deceptively tricky thing.

Potential solutions/workarounds include:

Switch to straight unittest for the coverage-data collection, and lose the benefits of parallelism for at least that particular test run.
Convert the entire test suite over from unittest[-parallel] to pytest, which I believe has already solved both the parallelization and coverage-collection problems, "separately and together" (as Perl's build configuration tool used to phrase it).

Converting to pytest has the potential to bring a whole host of benefits in addition to enabling coverage scanning, and would generally be a win on all levels. It's just a matter of putting in the effort to update all of the test code.

While pytest can be used as a test runner for unittest-based test suites, to get the benefits we'd want I believe it's necessary to actually write pytest tests, not simply run unittest-based tests through pytest.

lkk7 · 2024-07-16T15:56:47Z

Great idea, I've always liked pytest. the migration shouldn't be too difficult because the number of tests to be rewritten is tolerable, and a lot of them are automatically generated based on the `.dot' files. There are even automated tools that could help (but will probably fail because our tests are messy).

Let me know if you'll be working on the migration and coverage, or if I should do it. It would be bad to duplicate the effort.

ferdnyc · 2024-07-16T16:12:04Z

@lkk7 I'll take a first stab at it, and make it a goal to open a WIP PR early in the process. That way, you & anyone else can pitch in & share your thoughts. (And hopefully, flag me down before I get too far down any wrong paths.)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Test suite relies too heavily on dot_parser, fails to test Python API logic #385

Test suite relies too heavily on dot_parser, fails to test Python API logic #385

Uh oh!

Uh oh!

Uh oh!

Test suite relies too heavily on dot_parser, fails to test Python API logic #385

Test suite relies too heavily on dot_parser, fails to test Python API logic #385

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!