Skip to content

Reorganize tests#329

Merged
asmacdo merged 4 commits intocon:mainfrom
asmacdo:reorganize-tests
Dec 12, 2025
Merged

Reorganize tests#329
asmacdo merged 4 commits intocon:mainfrom
asmacdo:reorganize-tests

Conversation

@asmacdo
Copy link
Member

@asmacdo asmacdo commented Oct 21, 2025

Reorganize tests to match source structure

This PR reorganizes the test directory to better reflect the source code structure after the CLI refactoring.

What moved where

Original New Location
test/test_execution.py test/duct_main/test_execution.py
test/test_aggregation.py test/duct_main/test_aggregation.py
test/test_e2e.py test/duct_main/test_e2e.py
test/test_log_paths.py test/duct_main/test_log_paths.py
test/test_prepare_outputs.py test/duct_main/test_prepare_outputs.py
test/test_report.py test/duct_main/test_report.py
test/test_tailpipe.py test/duct_main/test_tailpipe.py
test/test_arg_parsing.py test/test_cli.py
test/test_plot_humanization.py test/test_plot.py
test/test_validation.py Split (see below)
test/test_suite.py Split (see below)

Files split into multiple destinations

test/test_validation.py (deleted):

  • test_assert_num_*test/duct_main/test_duct_utils.py (new file)
  • test_sample_*_intervaltest/duct_main/test_execution.py

test/test_suite.py (deleted):

  • TestSuiteHelperstest/test_cli.py
  • TestPPrint, TestPPrintHumanizationtest/test_pprint.py (new file)
  • TestPlotMatplotlibtest/test_plot.py
  • TestLStest/test_ls.py

Actual code changes (not just moves)

  1. test/duct_main/test_execution.py: Updated TEST_SCRIPT_DIR path from Path(__file__).with_name("data") to Path(__file__).parent.parent / "data" (required after moving to subdirectory)

  2. test/duct_main/test_e2e.py: Same TEST_SCRIPT_DIR path update

  3. test/duct_main/test_tailpipe.py: Same path update for cat_to_err.py script reference

  4. test_sample_equal_greater_than_report_interval: Renamed to test_sample_greater_than_report_interval (grammar fix)

🤖 Generated with Claude Code

@asmacdo asmacdo marked this pull request as draft October 21, 2025 18:03
@codecov
Copy link

codecov bot commented Oct 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.24%. Comparing base (7f5d63b) to head (383368e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #329   +/-   ##
=======================================
  Coverage   91.24%   91.24%           
=======================================
  Files           8        8           
  Lines        1005     1005           
  Branches      131      131           
=======================================
  Hits          917      917           
  Misses         67       67           
  Partials       21       21           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@asmacdo asmacdo mentioned this pull request Oct 27, 2025
4 tasks
@con con deleted a comment from mergify bot Dec 9, 2025
@asmacdo asmacdo marked this pull request as ready for review December 9, 2025 19:41
Copilot AI review requested due to automatic review settings December 9, 2025 19:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reorganizes the test directory structure to align with the source code structure following the CLI refactoring. Tests related to the duct_main module have been moved into a test/duct_main/ subdirectory, and several test files have been split or consolidated for better organization.

Key Changes

  • Moved 7 test files into test/duct_main/ subdirectory to mirror source structure
  • Split test_validation.py and test_suite.py into multiple focused test files
  • Consolidated test_plot_humanization.py into test_plot.py
  • Updated file path references to account for new directory structure
  • Renamed test function for improved clarity

Reviewed changes

Copilot reviewed 11 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
test/test_validation.py Deleted; tests split between test/duct_main/test_duct_utils.py and test/duct_main/test_execution.py
test/test_suite.py Deleted; tests distributed to test/test_cli.py, test/test_pprint.py, test/test_plot.py, and test/test_ls.py
test/test_plot_humanization.py Deleted; tests merged into test/test_plot.py
test/test_pprint.py New file containing pprint tests extracted from test_suite.py
test/test_plot.py New file merging plot tests from test_suite.py and test_plot_humanization.py
test/test_ls.py Updated with TestLS class from test_suite.py
test/test_cli.py Updated with TestSuiteHelpers class from test_suite.py
test/duct_main/test_execution.py Moved from test/; added interval validation tests; updated TEST_SCRIPT_DIR path
test/duct_main/test_e2e.py Moved from test/; updated TEST_SCRIPT_DIR path
test/duct_main/test_tailpipe.py Moved from test/; updated cat_to_err.py script path
test/duct_main/test_duct_utils.py New file containing assert_num tests from test_validation.py
test/duct_main/test_report.py New file (content moved from test/)
test/duct_main/test_prepare_outputs.py New file (content moved from test/)
test/duct_main/test_log_paths.py New file (content moved from test/)
test/duct_main/test_aggregation.py New file (content moved from test/)
Comments suppressed due to low confidence (2)

test/test_cli.py:63

  • Typo in method name: ssert_not_called() should be assert_not_called(). This typo was copied from the original test/test_suite.py file during the reorganization.
    test/test_cli.py:6
  • Module 'unittest' is imported with both 'import' and 'import from'.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@asmacdo asmacdo marked this pull request as draft December 9, 2025 20:11
@asmacdo
Copy link
Member Author

asmacdo commented Dec 9, 2025

Needs another rebase-- but lets wait until #333 merges so we can do all at once.

asmacdo and others added 4 commits December 11, 2025 16:04
- Rename test/duct/ → test/duct_main/ (mirrors duct_main.py)
- Rename test_arg_parsing.py → test_cli.py (consolidate CLI tests)
- Rename test_plot_humanization.py → test_plot.py (consolidate plot tests)

This organization better reflects the source code structure after the
CLI refactoring that split __main__.py into duct_main.py and cli.py.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move interval validation tests → test/duct_main/test_execution.py
  (tests execute() validation in duct_main.py)
- Move assert_num tests → test/duct_main/test_duct_utils.py
  (tests duct_main.py utility functions)
- Delete test_validation.py after migrating all content

This better organizes tests by the layer they're testing:
duct_main execution validation vs duct_main internal utilities.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- TestSuiteHelpers -> test/test_cli.py
- TestPPrint, TestPPrintHumanization -> test/test_pprint.py (new file)
- TestPlotMatplotlib -> test/test_plot.py
- TestLS -> test/test_ls.py

Updated to use pytest.importorskip for optional dependencies (yaml, pyout, matplotlib)
and tmp_path fixtures instead of mock_open where appropriate.
Changed TEST_SCRIPT_DIR from Path(__file__).with_name("data") to
Path(__file__).parent.parent / "data" since tests are now in
test/duct_main/ subdirectory but data remains in test/data/.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 15 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@asmacdo asmacdo marked this pull request as ready for review December 11, 2025 22:47
@asmacdo
Copy link
Member Author

asmacdo commented Dec 11, 2025

@CodyCBakerPhD this should be the last big one for a little bit, if youve got the bandwidth to have a look

@CodyCBakerPhD
Copy link
Contributor

For PRs like this I trust the coverage report more than anything

But if there were any tests that don't contribute to coverage (perhaps they just showcase an edge configuration) then I trust you or the AI did not lose them

Otherwise generally prefer to do this kind of thing over the course of several smaller PRs - what I would do is create a 'master branch' like this one to use as a reference (with no PR), but then separate branches/PR per new target file to make it easier to check that all methods were strictly moved

@CodyCBakerPhD
Copy link
Contributor

(I will probably do something like this eventually with nwb2bids)

@asmacdo
Copy link
Member Author

asmacdo commented Dec 12, 2025

Thanks @CodyCBakerPhD. Yeah I wish I had done it that way in retrospect-- in addition to a painful review the rebasing was awful (I had the AI recreate the commits)

Hit me up for nwb2bids, I owe you some review!

@asmacdo asmacdo merged commit 030abb1 into con:main Dec 12, 2025
21 checks passed
@CodyCBakerPhD
Copy link
Contributor

in addition to a painful review the rebasing was awful (I had the AI recreate the commits)

Very true - I also usually plan these types of refactors (and big linting ones) whenever possible to coincide with periods of time when there are no other active branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants