Update test_validate_nwb_path_grouping test

[jwodder]

The test in question was failing because the validate command was exiting nonzero.

@TheChymera Seeing as the sample file was expected to fail validation, exactly why was the command expected to exit successfully, and what changed?

[codecov]

Codecov Report

Base: 88.29% // Head: 88.27% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (26467d1) compared to base (c66fd18).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
- Coverage   88.29%   88.27%   -0.03%     
==========================================
  Files          73       73              
  Lines        8800     8800              
==========================================
- Hits         7770     7768       -2     
- Misses       1030     1032       +2     

Flag	Coverage Δ
unittests	88.27% <100.00%> (-0.03%)	⬇️

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

Impacted Files	Coverage Δ
dandi/cli/tests/test_cmd_validate.py	100.00% <100.00%> (ø)
dandi/cli/cmd_validate.py	71.59% <0.00%> (-2.28%)	⬇️
dandi/organize.py	82.81% <0.00%> (-0.45%)	⬇️
dandi/download.py	88.38% <0.00%> (+0.33%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TheChymera]

@jwodder

Seeing as the sample file was expected to fail validation

It wasn't expected to fail, it was just expected to give a warning which is asserted 2 lines later. We only fail on ERRORs not on WARNINGs, and what happened is that where there used to be a warning there is now an error.

As for why it fails, I am not sure, we did not change our code in that period, nor did obvious packages that might be involved in NWB validation (nwbinspector, pynwb) get new releases in that time period (latest releases from october).

[yarikoptic]

@TheChymera Seeing as the sample file was expected to fail validation, exactly why was the command expected to exit successfully, and what changed?

it was not expected to fail (I guess the docstring is misleading/need to be fixed if that is what gave you idea that it should fail). It is the upgrade of nwbinspector from 0.4.17 to 0.4.19 is what makes that test to fail...

running that validation manually brings up the reason:

❯ python -m pytest -s -v -k test_validate_nwb_path_grouping
====================================================== test session starts ======================================================
platform linux -- Python 3.10.7, pytest-7.1.2, pluggy-1.0.0 -- /home/yoh/proj/dandi/dandi-cli-master/venvs/dev3/bin/python
cachedir: .pytest_cache
rootdir: /home/yoh/proj/dandi/dandi-cli-master, configfile: tox.ini
plugins: mock-3.8.2, cov-3.0.0
collected 627 items / 626 deselected / 1 selected                                                                               

dandi/cli/tests/test_cmd_validate.py::test_validate_nwb_path_grouping 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> PDB set_trace >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> /home/yoh/proj/dandi/dandi-cli-master/dandi/cli/tests/test_cmd_validate.py(51)test_validate_nwb_path_grouping()
-> assert r.exit_code == 0
(Pdb) p simple3_nwb
'/home/yoh/.tmp/pytest-of-yoh/pytest-325/simple20/simple2.nwb'
(Pdb) 
[1]  + 1838094 suspended  python -m pytest -s -v -k test_validate_nwb_path_grouping
❯ dandi validate /home/yoh/.tmp/pytest-of-yoh/pytest-325/simple20/simple2.nwb
[NWBI.check_subject_id_exists] /home/yoh/.tmp/pytest-of-yoh/pytest-325/simple20/simple2.nwb — subject_id is missing.

so I guess nwbinspector started to demand subject_id to be specified . more specifically it was NeurodataWithoutBorders/nwbinspector#303 released in 0.4.18. It made the missing subject_id to be not a warning (does not result in error exit) but an error (error exit). I will leave it to @TheChymera to decide on what would be the most appropriate fix -- make simple3_nwb legit and tune up test next conditions, or something else.

[TheChymera]

yeah, let's just fix the test rather than assert it to be broken :3

[yarikoptic]

ok, please fix the test the way you want to have it fixed (as we briefly discussed -- add more of clearly failing validation files and assess groupping).For now to proceed let's just merge -- we just need to keep in mind that it would be failing with prior versions of nwbinspector

[CodyCBakerPhD]

we just need to keep in mind that it would be failing with prior versions of nwbinspector

Keep in mind too that since #1108, any time validation or upload is done with an active internet connection the latest version of the inspector becomes a requiment, so that should be pretty safe.

so I guess nwbinspector started to demand subject_id to be specified .

That's one of the oldest checks that even predates the inspector. The failing test case should have always been capturing that case (that is, true expected behavior is dandi validate should fail on any NWB file missing a subject_id)

more specifically it was NeurodataWithoutBorders/nwbinspector#303 released in 0.4.18. It made the missing subject_id to be not a warning (does not result in error exit) but an error (error exit).

To be clear, when I left #1108, if anything was ever returned by this line: https://github.com/dandi/dandi-cli/blob/master/dandi/files/bases.py#L504-L509 (hence termed 'errors', all) then it should have prevented validation. Strictly speaking, no one should have ever seen anything below Importance.CRITICAL as an output of that call due to the use of importance_threshold. You should keep that in mind as you start introducing more and more of this warning vs. error type stuff (which all came in well after #1108), so that if you do want to return the entire NWB Inspector report through your new dandi validation, you'll need to relax that threshold to allow checks that actually have a true configured importance below that.

Note also the actual filtering of checks based on configured importance was not affected by the issue and hotfix of NeurodataWithoutBorders/nwbinspector#303, which was an unfortunate issue in the display of the results. Put simply, true underlying importance was always used correctly under the hood, especially here in DANDI, but the importance attached to the returned message was from the default non-configured importance and I guess that caused an issue through the interaction with the new warning/error classification (which I've not had time to look through in detail). Sorry about not catching that sooner.