test: restructure suite and speed up CI#96
Merged
Conversation
Contributor
Reviewer's GuideRestructures the test suite into unit/integration/visualization layers with markers, adds a proper CLI entrypoint and lazy submodule loading, introduces coverage tooling and a Makefile coverage target, speeds up or marks slow tests while adding new tests around CLI, pipeline launcher, data loaders, tasks, utils, and visualization, and fixes environment dependencies (brainpy vs brainx) and CI pytest configuration. Sequence diagram for the new canns CLI entrypoint behaviorsequenceDiagram
actor User
participant Shell
participant cann_script as cann_console_script
participant main as canns_main
participant Launcher as pipeline_launcher
participant ASA as pipeline_asa
participant Gallery as pipeline_gallery
participant GUI as pipeline_asa_gui
participant Metadata as importlib_metadata
participant VersionModule as canns_version_module
User->>Shell: type cann [--version|--asa|--gallery|--gui]
Shell->>cann_script: execute entrypoint
cann_script->>main: main(argv)
alt --version flag
main->>Metadata: version(canns)
alt version lookup ok
Metadata-->>main: installed_version
main-->>User: print installed_version
else version lookup fails
main->>VersionModule: import __version__
alt module has __version__
VersionModule-->>main: __version__
main-->>User: print __version__
else fallback
main-->>User: print unknown
end
end
main-->>Shell: return 0
else --gui flag
main->>GUI: gui_main()
GUI-->>main: exit_code
main-->>Shell: return exit_code
else --gallery flag
main->>Gallery: gallery_main()
Gallery-->>main: complete
main-->>Shell: return 0
else --asa flag
main->>ASA: asa_main()
ASA-->>main: complete
main-->>Shell: return 0
else no flags
main->>Launcher: launcher_main()
Launcher-->>main: complete
main-->>Shell: return 0
end
Class diagram for canns lazy imports and CLI entrypointclassDiagram
class CannsPackage {
+set _LAZY_SUBMODULES
+__all__
+__getattr__(name str) Module
}
class AnalyzerModule
class DataModule
class ModelsModule
class PipelineModule
class TrainerModule
class UtilsModule
CannsPackage ..> AnalyzerModule : lazy import analyzer
CannsPackage ..> DataModule : lazy import data
CannsPackage ..> ModelsModule : lazy import models
CannsPackage ..> PipelineModule : lazy import pipeline
CannsPackage ..> TrainerModule : lazy import trainer
CannsPackage ..> UtilsModule : lazy import utils
class CannsMain {
+main(argv Sequence~str~) int
}
class PipelineLauncher {
+main() int
}
class PipelineASA {
+main() int
}
class PipelineGallery {
+main() void
}
class PipelineASA_GUI {
+main() int
}
CannsMain ..> PipelineLauncher : calls when no flags
CannsMain ..> PipelineASA : calls when --asa
CannsMain ..> PipelineGallery : calls when --gallery
CannsMain ..> PipelineASA_GUI : calls when --gui
class LintTool {
+main() void
}
LintTool : env_var_CI bool
LintTool : fix bool
LintTool : run_codespell()
LintTool : run_ruff_check(fix bool)
LintTool : run_ruff_format(check_only bool)
CannsPackage <.. CannsMain : package entrypoint script
LintTool <.. CannsPackage : developer tooling
Flow diagram for the restructured pytest suite and markersflowchart TD
A[tests directory] --> B[unit tests]
A --> C[integration tests]
A --> D[visualization tests]
B --> B1[tests/unit/...]
C --> C1[tests/integration/...]
D --> D1[tests/visualization/...]
subgraph Markers
M1[unit tests\nno special marker]
M2[integration tests\nmarker: integration]
M3[visualization tests\nmarker: visualization]
M4[slow tests across layers\nmarker: slow]
end
B1 --> M1
C1 --> M2
D1 --> M3
M2 --> M4
M3 --> M4
subgraph PytestConfiguration
P1[python_files: test_*.py]
P2[testpaths: tests]
P3[markers registered\n integration, visualization, slow]
P4[addopts: --assert=plain -ra]
end
A --> PytestConfiguration
subgraph LocalRun
L1[uv run pytest]
L1 --> L2[run all tests\nincluding slow]
end
subgraph CISuggestedRun
CI1[uv run pytest -m not slow]
CI1 --> CI2[run all tests\nexcept slow]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In the new CLI entrypoint (
canns.__main__), only the GUI path propagates a return code while the ASA and gallery paths always return 0; consider consistently returning the subcommand’s exit code (or at least handling non-zero returns) so failures in those entrypoints surface correctly to callers and CI. - The lazy import mechanism in
canns.__init__implements__getattr__but not__dir__or an updated__all__; adding these for the lazy submodules would improve discoverability and type-checker/IDE support forcanns.analyzer,canns.models, etc.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new CLI entrypoint (`canns.__main__`), only the GUI path propagates a return code while the ASA and gallery paths always return 0; consider consistently returning the subcommand’s exit code (or at least handling non-zero returns) so failures in those entrypoints surface correctly to callers and CI.
- The lazy import mechanism in `canns.__init__` implements `__getattr__` but not `__dir__` or an updated `__all__`; adding these for the lazy submodules would improve discoverability and type-checker/IDE support for `canns.analyzer`, `canns.models`, etc.
## Individual Comments
### Comment 1
<location> `pyproject.toml:274` </location>
<code_context>
+exclude_lines = [
+ "pragma: no cover",
+ "if TYPE_CHECKING:",
+ "if __name__ == .__main__.:",
+ "raise NotImplementedError",
+]
</code_context>
<issue_to_address>
**issue (testing):** Coverage exclude pattern for __main__ guard looks incorrect and likely won’t match the intended line.
`exclude_lines` uses regex matching, so `"if __name__ == .__main__.:"` is unlikely to match a typical guard like `if __name__ == "__main__":`. The `.` characters will match any character and the quoting doesn’t align with the usual pattern. If you want to ignore standard `__main__` guards, consider a more accurate regex such as `"if __name__ == ['\"]__main__['\"]:"` or a simpler literal match that reflects the actual line format.
</issue_to_address>
### Comment 2
<location> `tests/integration/analyzer/experimental_data/test_experimental_data_cann2d.py:156-165` </location>
<code_context>
+from typing import TYPE_CHECKING
# Version information
try:
</code_context>
<issue_to_address>
**issue (testing):** Swallowing exceptions in `test_decode_circular_coordinates` can mask regressions.
Catching all exceptions and only logging them means real failures in `tda_vis` or `decode_circular_coordinates` will be silently ignored. If you need to handle known platform- or data-specific issues, please use `pytest.skip` / `pytest.xfail` under a concrete condition or catch a specific exception type. Otherwise, let exceptions surface so the test fails when decoding is broken.
</issue_to_address>
### Comment 3
<location> `tests/integration/analyzer/experimental_data/test_asa_experimental_data.py:135-144` </location>
<code_context>
assert xx is not None and yy is not None and tt is not None
+@pytest.mark.slow
def test_tda_decode_and_cohomap():
# grid_data = load_grid_data()
</code_context>
<issue_to_address>
**suggestion (testing):** The combined `integration` + `slow` marking may exclude this important TDA/decoding path from normal CI runs.
Since this test hits a high-level ASA/TDA + decoding pipeline, having it marked as both `integration` and `slow` means it won’t run in the default `-m "not slow"` CI configuration described in the PR. Consider adding a lighter-weight non-slow test that still exercises the decoding + plotting config behavior (e.g., reduced neurons/timepoints or fewer `n_points` values), and keep this heavier version as an explicitly slow test.
</issue_to_address>
### Comment 4
<location> `tests/unit/test_lazy_imports.py:10-17` </location>
<code_context>
+ sys.modules.pop(name, None)
+
+
+def test_lazy_imports():
+ _clear_canns_modules()
+
+ import canns
+
+ assert "canns.models" not in sys.modules
+ _ = canns.models
+ assert "canns.models" in sys.modules
</code_context>
<issue_to_address>
**suggestion (testing):** Extend lazy import test to cover all declared lazy submodules.
Currently this only verifies lazy import for `canns.models`, but `__init__` declares several lazy submodules (`analyzer`, `data`, `models`, `pipeline`, `trainer`, `utils`). Consider parametrizing the test over this set and asserting that each attribute access (e.g. `canns.analyzer`) causes the corresponding `canns.<name>` module to appear in `sys.modules`.
Suggested implementation:
```python
import sys
import pytest
```
```python
@pytest.mark.parametrize(
"submodule",
["analyzer", "data", "models", "pipeline", "trainer", "utils"],
)
def test_lazy_imports(submodule):
_clear_canns_modules()
import canns
full_name = f"canns.{submodule}"
assert full_name not in sys.modules
# Access the lazy attribute to trigger import
_ = getattr(canns, submodule)
assert full_name in sys.modules
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| exclude_lines = [ | ||
| "pragma: no cover", | ||
| "if TYPE_CHECKING:", | ||
| "if __name__ == .__main__.:", |
Contributor
There was a problem hiding this comment.
issue (testing): Coverage exclude pattern for main guard looks incorrect and likely won’t match the intended line.
exclude_lines uses regex matching, so "if __name__ == .__main__.:" is unlikely to match a typical guard like if __name__ == "__main__":. The . characters will match any character and the quoting doesn’t align with the usual pattern. If you want to ignore standard __main__ guards, consider a more accurate regex such as "if __name__ == ['\"]__main__['\"]:" or a simpler literal match that reflects the actual line format.
Comment on lines
156
to
+165
| try: | ||
| persistence_result = tda_vis(embed_spikes, tda_config) | ||
|
|
||
| # Test coordinate decoding | ||
| decoding_result = decode_circular_coordinates( | ||
| persistence_result=persistence_result, | ||
| embed_data=embed_data, | ||
| real_ground=True, | ||
| real_of=True, | ||
| save_path=None | ||
| save_path=None, |
Contributor
There was a problem hiding this comment.
issue (testing): Swallowing exceptions in test_decode_circular_coordinates can mask regressions.
Catching all exceptions and only logging them means real failures in tda_vis or decode_circular_coordinates will be silently ignored. If you need to handle known platform- or data-specific issues, please use pytest.skip / pytest.xfail under a concrete condition or catch a specific exception type. Otherwise, let exceptions surface so the test fails when decoding is broken.
Comment on lines
+135
to
+144
| @pytest.mark.slow | ||
| def test_tda_decode_and_cohomap(): | ||
| # grid_data = load_grid_data() | ||
| # if grid_data is None: | ||
| grid_data = create_mock_spike_data( | ||
| num_neurons=50, | ||
| num_timepoints=3000, | ||
| density=2.0, | ||
| num_neurons=20, | ||
| num_timepoints=600, | ||
| density=1.2, | ||
| structured=True, | ||
| duration=30.0, | ||
| duration=10.0, |
Contributor
There was a problem hiding this comment.
suggestion (testing): The combined integration + slow marking may exclude this important TDA/decoding path from normal CI runs.
Since this test hits a high-level ASA/TDA + decoding pipeline, having it marked as both integration and slow means it won’t run in the default -m "not slow" CI configuration described in the PR. Consider adding a lighter-weight non-slow test that still exercises the decoding + plotting config behavior (e.g., reduced neurons/timepoints or fewer n_points values), and keep this heavier version as an explicitly slow test.
Comment on lines
+10
to
+17
| def test_lazy_imports(): | ||
| _clear_canns_modules() | ||
|
|
||
| import canns | ||
|
|
||
| assert "canns.models" not in sys.modules | ||
| _ = canns.models | ||
| assert "canns.models" in sys.modules |
Contributor
There was a problem hiding this comment.
suggestion (testing): Extend lazy import test to cover all declared lazy submodules.
Currently this only verifies lazy import for canns.models, but __init__ declares several lazy submodules (analyzer, data, models, pipeline, trainer, utils). Consider parametrizing the test over this set and asserting that each attribute access (e.g. canns.analyzer) causes the corresponding canns.<name> module to appear in sys.modules.
Suggested implementation:
import sys
import pytest@pytest.mark.parametrize(
"submodule",
["analyzer", "data", "models", "pipeline", "trainer", "utils"],
)
def test_lazy_imports(submodule):
_clear_canns_modules()
import canns
full_name = f"canns.{submodule}"
assert full_name not in sys.modules
# Access the lazy attribute to trigger import
_ = getattr(canns, submodule)
assert full_name in sys.modules
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test
Summary by Sourcery
Restructure the test suite into unit, integration, and visualization categories with markers, speed up slower tests, and improve CLI, packaging, and developer tooling for coverage and linting.
New Features:
cannsCLI with subcommands for ASA, gallery, GUI, and version reporting.Bug Fixes:
cannsconsole script to the new CLI entry point instead of the previousmasterattribute.brainpy[cpu]instead ofbrainx[cpu].Enhancements:
Build:
coveragetarget that runs pytest with coverage reporting and ensure formatting is checked (not auto-fixed) in CI via the lint helper.CI:
Tests: