Skip to content

Split test suite into unit/ and integration/ subdirectories#615

Open
jishanahmed-shaikh wants to merge 2 commits into
mllam:mainfrom
jishanahmed-shaikh:split-unit-integration-tests
Open

Split test suite into unit/ and integration/ subdirectories#615
jishanahmed-shaikh wants to merge 2 commits into
mllam:mainfrom
jishanahmed-shaikh:split-unit-integration-tests

Conversation

@jishanahmed-shaikh

@jishanahmed-shaikh jishanahmed-shaikh commented Apr 20, 2026

Copy link
Copy Markdown

Describe your changes

Split the flat tests/ directory into tests/unit/ and tests/integration/ to make the distinction between fast isolated tests and slow S3-dependent tests visible in the file tree.

  • tests/unit/: test_imports, test_config, test_cli, test_time_slicing, test_train_model_warnings, test_clamping (refactored to use DummyDatastore)
  • tests/integration/: test_datastores, test_graph_creation, test_training, test_datasets, test_plotting, test_plot_graph
  • conftest.py holds the S3 download fixture and init_datastore_example; conftest.py is stripped to shared env setup only
  • test_clamping.py refactored to use DummyDatastore instead of MDPDatastore — no S3 dependency
  • pyproject.toml: added [tool.pytest.ini_options] with testpaths and integration marker
  • CI: split single pytest step into separate unit and integration runs

Issue Link

closes #599

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (Addition or improvements to documentation)
  • Maintenance

Checklist before requesting a review

  • My branch is up-to-date with the target branch
  • I have performed a self-review of my code
  • For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
  • I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
  • I have updated the README to cover introduced code changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have given the PR a name that clearly describes the change, written in imperative form
  • I have requested a reviewer and an assignee

Checklist for reviewers

  • the code is readable
  • the code is well tested
  • the code is documented (including return types and parameters)
  • the code is easy to maintain

Author checklist after completed review

  • I have added a line to the CHANGELOG describing this change, in a section reflecting type of change (maintenance)

@sadamov sadamov self-requested a review April 20, 2026 19:30
@sadamov sadamov self-assigned this Apr 20, 2026
@sadamov sadamov added the cicd label Apr 20, 2026

@sadamov sadamov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @jishanahmed-shaikh, direction is right. A few things to address before merge.

Top-level:

  • uv.lock (+5079 lines) is unrelated to the test split, please remove and open a separate PR if committing the lockfile is desired.
  • Delete tests/unit/conftest.py entirely. ensure_rank_zero and WANDB_MODE are already in the root tests/conftest.py and pytest picks up parent conftests automatically.
  • --tb=short is passed to the unit run but not the integration run, please align.
  • Branch is CONFLICTING with main, needs a rebase.

Inline comments below.

Comment thread .github/workflows/install-and-test.yml Outdated
Comment thread pyproject.toml Outdated
Comment thread tests/integration/conftest.py Outdated
Comment thread tests/unit/test_clamping.py Outdated
Comment thread tests/unit/test_clamping.py Outdated
Comment thread tests/unit/__init__.py Outdated
Comment thread tests/integration/__init__.py Outdated
Comment thread CHANGELOG.md Outdated
jishanahmed-shaikh added a commit to jishanahmed-shaikh/neural-lam that referenced this pull request Apr 26, 2026
- Remove uv.lock (unrelated to this PR)
- Delete tests/unit/conftest.py (root conftest covers WANDB_MODE and
  ensure_rank_zero; pytest picks up parent conftests automatically)
- Strip tests/integration/conftest.py to S3/datastore code only,
  removing duplicated WANDB_MODE and ensure_rank_zero blocks
- Move DATASTORES.setdefault registration to root tests/conftest.py
  so the registry is touched in one place
- Remove DATASTORES mutation from tests/unit/test_clamping.py
- Fix stale comment in test_clamping.py (drop hardcoded feature count)
- Split CI into three steps: unit tests, integration tests, doctests;
  align --tb=short across both test steps; run doctests against
  neural_lam/ explicitly so they are not lost
- Drop unused 'integration' pytest marker from pyproject.toml
- Fix CHANGELOG entry to link PR mllam#615 instead of issue mllam#599
- Truncate tests/unit/__init__.py and tests/integration/__init__.py
  to 0 bytes (match existing tests/__init__.py convention)

Co-authored-by: Kiro <kiro@amazon.com>
@jishanahmed-shaikh jishanahmed-shaikh force-pushed the split-unit-integration-tests branch from 4d7f2bf to 6112087 Compare April 26, 2026 07:45
- Move test_imports, test_config, test_cli, test_time_slicing,
  test_train_model_warnings into tests/unit/
- Refactor test_clamping to use DummyDatastore instead of
  MDPDatastore and move to tests/unit/
- Move test_datastores, test_graph_creation, test_training,
  test_datasets, test_plotting, test_plot_graph into tests/integration/
- Add tests/integration/conftest.py with S3 fixture and
  init_datastore_example; strip tests/conftest.py to shared env setup
- Add tests/unit/conftest.py with no S3 dependencies
- Register pytest marker 'integration' and testpaths in pyproject.toml
- Split CI pytest step into unit and integration runs
- Remove uv.lock (unrelated to this PR)
- Delete tests/unit/conftest.py (root conftest covers WANDB_MODE and
  ensure_rank_zero; pytest picks up parent conftests automatically)
- Strip tests/integration/conftest.py to S3/datastore code only,
  removing duplicated WANDB_MODE and ensure_rank_zero blocks
- Move DATASTORES.setdefault registration to root tests/conftest.py
  so the registry is touched in one place
- Remove DATASTORES mutation from tests/unit/test_clamping.py
- Fix stale comment in test_clamping.py (drop hardcoded feature count)
- Split CI into three steps: unit tests, integration tests, doctests;
  align --tb=short across both test steps; run doctests against
  neural_lam/ explicitly so they are not lost
- Drop unused 'integration' pytest marker from pyproject.toml
- Fix CHANGELOG entry to link PR mllam#615 instead of issue mllam#599
- Truncate tests/unit/__init__.py and tests/integration/__init__.py
  to 0 bytes (match existing tests/__init__.py convention)
@jishanahmed-shaikh jishanahmed-shaikh force-pushed the split-unit-integration-tests branch from 6112087 to 4a66af6 Compare April 26, 2026 08:28
@sadamov

sadamov commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Hi @jishanahmed-shaikh, are you still interested in driving this one? It would need a rebase, the inline comments above addressed, and the ~7 new test files added to tests/ since (e.g. test_custom_loggers.py, test_utils.py, test_probabilistic_objectives.py) classified as unit vs integration.

I'd also want to discuss the unit/integration split as a convention at a dev meeting before enforcing it, so even with a rebase ready the PR might sit a bit. Let me know if you still have bandwidth, otherwise happy to close until someone picks it up. Thanks for the proposal either way!

@jishanahmed-shaikh

Copy link
Copy Markdown
Author

Hi @jishanahmed-shaikh, are you still interested in driving this one? It would need a rebase, the inline comments above addressed, and the ~7 new test files added to tests/ since (e.g. test_custom_loggers.py, test_utils.py, test_probabilistic_objectives.py) classified as unit vs integration.

I'd also want to discuss the unit/integration split as a convention at a dev meeting before enforcing it, so even with a rebase ready the PR might sit a bit. Let me know if you still have bandwidth, otherwise happy to close until someone picks it up. Thanks for the proposal either way!

Hi, thanks for checking in.

I'm still interested in working on this, but due to other commitments I wouldn't be able to dedicate the necessary bandwidth for another 2 weeks. Realistically, I'd want enough time to properly rebase, address the review comments, and go through the newer test files and classification work.

If that timeline works for the project, I'd be happy to pick it back up then. Otherwise, I completely understand if you'd prefer to close the PR for now and have someone else take it on.

Thanks again for reaching out, and I appreciate the flexibility either way.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Separate unit tests from integration tests

2 participants