Skip to content

[ENH] Add conditional test decorators to recently added tests #997#1001

Open
ANANYA542 wants to merge 2 commits intosktime:mainfrom
ANANYA542:enh/conditional-test-decorators-997-clean
Open

[ENH] Add conditional test decorators to recently added tests #997#1001
ANANYA542 wants to merge 2 commits intosktime:mainfrom
ANANYA542:enh/conditional-test-decorators-997-clean

Conversation

@ANANYA542
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #997

What does this implement/fix? Explain your changes.

Adds @pytest.mark.skipif decorators using run_test_for_class and run_test_module_changed to recently added tests that were missing them. This ensures these tests only run on CI when the underlying class or module has changed, reducing unnecessary CI load.
Files changed (7):

  1. test_normal_mixture.py— added run_test_for_class(NormalMixture) to all 6 tests
  2. test_multiindex.py — added module-level pytestmark with run_test_module_changed("skpro.distributions") covering all 7 tests
  3. test_proba_basic.py— added run_test_module_changed("skpro.distributions") to 6 tests that were missing it (test_proba_subsetters_loc_iloc, test_proba_subsetters_at_iat, test_proba_index_coercion, test_proba_plotting, test_to_df_parametric, test_head_tail). Tests already decorated or unconditionally skipped were left unchanged.
  4. test_bandwidth.py — added module-level pytestmark with run_test_module_changed("skpro.regression") covering all 5 tests
  5. test_mapie_v1.py— added run_test_for_class(MAPIE_CLASSES) to all 3 tests, with MAPIE classes imported at module level for collection-time evaluation
  6. test_ondil.py— added run_test_for_class(OndilOnlineGamlss) to both tests
  7. test_pipeline_transform_chain.py — added run_test_for_class([Pipeline, ResidualDouble]) to the single test; also fixed CRLF → LF line endings

Does your contribution introduce a new dependency? If yes, which one?

no

What should a reviewer concentrate their feedback on?

  1. Correctness of the decorator choice: run_test_for_class for class-specific tests vs run_test_module_changed for module-level tests
  2. Whether pytestmark (module-level) vs individual decorators are used appropriately
  3. The test_mapie_v1.py approach of importing all 4 MAPIE classes at module level and grouping them into a MAPIE_CLASSES list for run_test_for_class

Did you add any tests for the change?

No new tests - this PR adds skip decorators to existing tests

Any other comments?

Tests that are already unconditionally skipped( test_proba_example, test_discreate_pmf_plotting - both marked with @pytest.mark.skip) were intentionally left without the conditional decorator as they are already skipped regardless.

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

@ANANYA542
Copy link
Copy Markdown
Contributor Author

@fkiraly please review the pr and let me know if some changes are required

)
from skpro.tests.test_switch import run_test_module_changed

pytestmark = pytest.mark.skipif(
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.

I would make this more specific to the containing modules

Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly 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! Just one change request above.

@ANANYA542
Copy link
Copy Markdown
Contributor Author

@fkiraly I have made the changes as requested if you could review once and let me know if this works? or need any further changes

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENH] Ensure conditional running of recently added tests

2 participants