Skip to content

TST: add tests for unsupported graph types in MST algorithms#8353

Merged
rossbar merged 2 commits intonetworkx:mainfrom
Aka2210:tst-mst-directed-raises
Nov 24, 2025
Merged

TST: add tests for unsupported graph types in MST algorithms#8353
rossbar merged 2 commits intonetworkx:mainfrom
Aka2210:tst-mst-directed-raises

Conversation

@Aka2210
Copy link
Copy Markdown
Contributor

@Aka2210 Aka2210 commented Nov 24, 2025

Summary

This PR adds tests to ensure that MST algorithms raise NetworkXNotImplemented on unsupported graph types.

Motivation

While reviewing the MST code in mst.py, I noticed that if the @not_implemented_for("directed") decorator is accidentally removed, the entire MST test suite still passes. In that situation, MST functions silently accept DiGraph (or MultiGraph for Borůvka) and return results, even though the documented behavior specifies that these graph types are not supported.

Although it is unlikely that the decorator will be removed intentionally—and such a change would typically be caught during code review—the lack of test coverage means this would not be detected by automated testing. To prevent this silent regression scenario, I added tests that verify:

  • All MST algorithms (Kruskal, Prim, and Borůvka) raise NetworkXNotImplemented on DiGraph.

  • Borůvka raises NetworkXNotImplemented on MultiGraph.

  • Kruskal and Prim continue to accept MultiGraph, as expected.

These tests ensure that the intended API contract is explicitly validated.

Notes

I am not entirely certain whether these tests are desirable or redundant, so I am submitting this PR to gather feedback. If the maintainers feel that these tests improve API contract coverage, they can be merged; otherwise, I am happy to revise or close the PR.

Thank you for reviewing!

Added tests ensuring that MST functions raise NetworkXNotImplemented
on DiGraph (default case) and on MultiGraph for Borůvka.
Copy link
Copy Markdown
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me -- Thanks!
The diff makes it hard to see that the first change is in class MinimumSpanningTreeTestBase so it gets run on all algos. The second change is in the class TestBoruvka so it only gets run for the boruvka algo.

Thanks @Aka2210

Copy link
Copy Markdown
Contributor

@amcandio amcandio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! You could also define a list of supported graphs in the base class and override that on the specific subclass

Copy link
Copy Markdown
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @Aka2210 !

@rossbar rossbar merged commit 1f71989 into networkx:main Nov 24, 2025
52 of 53 checks passed
@dschult dschult added this to the 3.6 milestone Nov 24, 2025
@Aka2210 Aka2210 deleted the tst-mst-directed-raises branch November 25, 2025 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants