Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use mixing tests for mocks #1136

Merged
merged 8 commits into from
Nov 15, 2024
Merged

Use mixing tests for mocks #1136

merged 8 commits into from
Nov 15, 2024

Conversation

h-mayorquin
Copy link
Collaborator

We have specific tests for the mock recording interface that duplicates the tests on the mixing. This PR solves this by using the mixing tests for the mock (We already doing that in the ophys side).

This PR also separates some changes that I did on #1124 and should come before it.

@h-mayorquin h-mayorquin self-assigned this Nov 12, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review November 12, 2024 15:16
Copy link
Member

@pauladkisson pauladkisson 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 to me, but we should remove the 'old' Test classes before we merge right?

@h-mayorquin
Copy link
Collaborator Author

Looks good to me, but we should remove the 'old' Test classes before we merge right?

I remove the ones from the Old Recording Extractor. I don't really like the stub tests were done in the old tests. Since those were written I have written better machinery on the SpikeInterface side. I will update those in a separate PR though and then remove the old ones.

@pauladkisson
Copy link
Member

I see, so you'll remove those old tests after #1124 is ready to go?

@pauladkisson
Copy link
Member

Also looks like test_no_slash_in_name is failing...

@h-mayorquin
Copy link
Collaborator Author

h-mayorquin commented Nov 14, 2024

I see, so you'll remove those old tests after #1124 is ready to go?

Yes, if not before. I just don't want this to get in the way of #1124 as it would just be a cleaning/refactor PR and #1124 is building towards and enhancement coming from a user request.

@h-mayorquin
Copy link
Collaborator Author

Also looks like test_no_slash_in_name is failing...

Should be fixed now.

@pauladkisson
Copy link
Member

I see, so you'll remove those old tests after #1124 is ready to go?

Yes, if not before. I just don't want this to get in the way of #1124 as it would just be a cleaning/refactor PR and #1124 is building towards and enhancement coming from a user request.

Sounds good. As long as we have a clear plan to remove it, so it doesn't just end up hanging around with a comment saying it should be removed.

Copy link
Member

@pauladkisson pauladkisson left a comment

Choose a reason for hiding this comment

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

Tests are failing, but should pass after #1140 gets merged.

@h-mayorquin h-mayorquin merged commit 64fb9e0 into main Nov 15, 2024
40 checks passed
@h-mayorquin h-mayorquin deleted the use_common_tests_for_mocks branch November 15, 2024 03:33
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.58%. Comparing base (6960872) to head (eb1e1dd).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1136      +/-   ##
==========================================
- Coverage   90.60%   90.58%   -0.02%     
==========================================
  Files         129      128       -1     
  Lines        8174     8011     -163     
==========================================
- Hits         7406     7257     -149     
+ Misses        768      754      -14     
Flag Coverage Δ
unittests 90.58% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...c/neuroconv/tools/testing/data_interface_mixins.py 95.55% <ø> (-0.08%) ⬇️

... and 2 files with indirect coverage changes

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.

2 participants