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.

Sign up for GitHub

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

Fix for newer HDMF #1191

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Fix for newer HDMF #1191

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2eb1a74
Fix for newer HDMF
TheChymera Jan 23, 2023
9ad66c4
Better commenting and docstring
TheChymera Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions dandi/tests/test_files.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import subprocess
from typing import cast
from unittest.mock import ANY
import warnings

from dandischema.models import get_schema_version
import numpy as np
Expand Down Expand Up @@ -353,12 +354,22 @@ def test_validate_simple3_no_subject_id(simple3_nwb):


def test_validate_bogus(tmp_path):
"""
Notes
-----
* Intended to produce use-case for https://github.com/dandi/dandi-cli/issues/93
but it would be tricky, so it is more of a smoke test that
we do not crash
"""
path = tmp_path / "wannabe.nwb"
path.write_text("not really nwb")
# intended to produce use-case for https://github.com/dandi/dandi-cli/issues/93
# but it would be tricky, so it is more of a smoke test that
# we do not crash
errors = dandi_file(path).get_validation_errors()
# We need to catch this warning lest pytest interprets it as a failure:
# https://github.com/hdmf-dev/hdmf/issues/817#issuecomment-1399080575
with warnings.catch_warnings():
warnings.filterwarnings(
"ignore", message="HDF5IO was not fully initialized before close"
)
errors = dandi_file(path).get_validation_errors()
Copy link
Member

Choose a reason for hiding this comment

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

please add a comment on why needs to be filtered, point to hdmf issue.

PR should be accompanied with the revert of 6d065ec to ensure that we test it with current/buggy hdmf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commenting improved in 9ad66c4.

@yarikoptic
As for the second part, sorry if my explanation in the first post was not clear enough, but we specifically do not want to revert that commit. The issue is a real bug in 3.5.0, which we cannot easily circumvent in DANDI. This PR simply leverages the upstream patch, which will become available upon the next release, hopefully one we can ask for right now if you agree this has been properly addressed for our use case (I do).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think I got it. But is this the only place where such warning might be raised? If not, we might need to add it to https://github.com/dandi/dandi-cli/blob/master/tox.ini#L48 instead of to some other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this is the only test with that issue. Not sure if it's a good idea to move the catching to tox.ini — as that will not be caught when running straight-up pytest... or maybe I'm misunderstanding what you're suggesting? I predominantly use pytest directly to test locally, and none of the tox.ini errors ever popped up 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case upstream said a new patch release for this sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

confusingly enough pytest reads (as well) config from tox.ini, see https://docs.pytest.org/en/7.1.x/reference/customize.html#tox-ini , that is why you didn't see any error. Ok, if you say that it is the only test -- let's proceed.

# ATM we would get 2 errors -- since could not be open in two places,
# but that would be too rigid to test. Let's just see that we have expected errors
assert any(e.message.startswith("Unable to open file") for e in errors)
Expand Down