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

Simplify testing suite #509

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Simplify testing suite #509

merged 3 commits into from
Sep 17, 2024

Conversation

CodyCBakerPhD
Copy link
Contributor

While working on #507 noticed some needlessly complicated setup to a very particular corner of the testing suite

Replacing it all with a simple environment variable

@CodyCBakerPhD CodyCBakerPhD self-assigned this Sep 6, 2024
@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review September 6, 2024 19:20
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.90%. Comparing base (c7861f4) to head (b5aa045).
Report is 2 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #509      +/-   ##
==========================================
+ Coverage   79.53%   84.90%   +5.37%     
==========================================
  Files          48       48              
  Lines        1412     1398      -14     
==========================================
+ Hits         1123     1187      +64     
+ Misses        289      211      -78     
Files with missing lines Coverage Δ
src/nwbinspector/testing/__init__.py 100.00% <ø> (ø)
src/nwbinspector/testing/_testing.py 80.95% <ø> (-0.87%) ⬇️

... and 4 files with indirect coverage changes

Copy link
Contributor

@stephprince stephprince 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.

My only question is what is the case where the testing files get changed / need to be reuploaded to DANDI? Will that be done manually now since the update-testing-files.yml workflow was removed?

@CodyCBakerPhD
Copy link
Contributor Author

My only question is what is the case where the testing files get changed / need to be reuploaded to DANDI? Will that be done manually now since the update-testing-files.yml workflow was removed?

While we once had aspirations for it to be all automated, the reality is that process will continue as it always has - manually

Manually create a specific (usually old) environment and run some code that only works in that specific old environment in order to create the 'bad' example file for testing

But even that should really just be for new 'bad' examples - the previous ones can just be downloaded from DANDI and shouldn't really ever need to be replaced for any reason I know of

@stephprince
Copy link
Contributor

But even that should really just be for new 'bad' examples - the previous ones can just be downloaded from DANDI and shouldn't really ever need to be replaced for any reason I know of

Ok sounds good. Follow up question - since the nwbinspector test file dandiset is on the staging server (which from my understanding is not guaranteed to continue existing?), is there a backup of these files somewhere? I saw the dandiset said some files were from user submissions so I assume not all of them can be auto-generated from the package.

@CodyCBakerPhD
Copy link
Contributor Author

DANDI team would/should provide a big heads up on clearing any staging content or putting any such policy into place

At that point, if it happens, I'd recommend moving it to the main archive and publishing official persistent releases

@CodyCBakerPhD CodyCBakerPhD merged commit bdf3548 into dev Sep 17, 2024
38 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the simply_testing branch September 17, 2024 00:51
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.

3 participants