[ENH] refactor repetitive clone tests with pytest.mark.parametrize, fixes #170

[JahnaviDhanaSri]

Reference Issues/PRs

Fixes #170

What does this implement/fix? Explain your changes.

refactored repetitive clone tests with pytest.mark.parametrize

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

No

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions

• I've reviewed the project documentation on contributing
• I've added myself to the list of contributors.
• The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
  the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.

For code contributions

• Unit tests have been added covering code functionality
• Appropriate docstrings have been added (see documentation standards)
• New public functionality has been added to the API Reference

[fkiraly]

Thanks a lot!

Some comments:

• the tests test_clone_none_and_empty_array and test_clone_nan_sparse_matrix look like they could be merged
• we should not import sklearn on module level to ensure soft dependency isolation. Could you leave that in the tests?
• The sklearn tests do not look very similar, in fact the assertions are different, and the parameterization make the tests more difficult to read. Could you revert your changes to the sklearn tests?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.77%. Comparing base (306958d) to head (1b27254).
Report is 97 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   85.07%   85.77%   +0.70%     
==========================================
  Files          45       48       +3     
  Lines        3015     3311     +296     
==========================================
+ Hits         2565     2840     +275     
- Misses        450      471      +21     

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

[JahnaviDhanaSri]

Hi @fkiraly , i have made changes according to your comments please review it

[fkiraly]

Looks good! I will start the tests.

[fkiraly]

Just reviewing, and it seems the new tests do not exactly test the same:

• in the case of np.array, it is fine to use assert_array_equal to check for equality in value
• in the other two cases we need to check identity, as in the tests before, using is

The difference is whether the object in question is an np.ndarray.

[fkiraly]

could you ensure the logic is identical to what was there before?

That is, in the test using assert_array_equal vs is, using an if condition.

[fkiraly]

I fixed it quickly - was only a small thing - and added you to the contributors file.

Thanks a lot!

[JahnaviDhanaSri]

Thank you so much!