diff --git a/dandi/tests/fixtures.py b/dandi/tests/fixtures.py index 7a8b21998..3be463b18 100644 --- a/dandi/tests/fixtures.py +++ b/dandi/tests/fixtures.py @@ -447,6 +447,15 @@ def bids_dandiset(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDan return new_dandiset +@pytest.fixture() +def bids_dandiset_invalid(new_dandiset: SampleDandiset, bids_examples: str) -> SampleDandiset: + copytree( + os.path.join(bids_examples, "invalid_pet001"), + str(new_dandiset.dspath) + "/", + ) + return new_dandiset + + @pytest.fixture() def video_files(tmp_path): video_paths = [] diff --git a/dandi/tests/test_upload.py b/dandi/tests/test_upload.py index 27ee93c08..39cd611bb 100644 --- a/dandi/tests/test_upload.py +++ b/dandi/tests/test_upload.py @@ -181,9 +181,27 @@ def test_upload_sync_folder( text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt") -def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> None: +def test_upload_bids_invalid( + mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset +) -> None: iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") - bids_dandiset.upload(existing="forced") + # Does it fail when it should fail? + with pytest.raises(RuntimeError): + bids_dandiset_invalid.upload(existing="forced") + iter_upload_spy.assert_not_called() + # Does validation ignoring work? + bids_dandiset_invalid.upload(existing="forced", validation="ignore") + iter_upload_spy.assert_called() + # Check existence of assets: + dandiset = bids_dandiset_invalid.dandiset + dandiset.get_asset_by_path("dataset_description.json") + + +def test_upload_bids_validation_ignore( + mocker: MockerFixture, bids_dandiset: SampleDandiset +) -> None: + iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") + bids_dandiset.upload(existing="forced", validation="ignore") # Check whether upload was run iter_upload_spy.assert_called() # Check existence of assets: @@ -196,11 +214,9 @@ def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> No dandiset.get_asset_by_path("sub-Sub1/anat/sub-Sub1_T1w.nii.gz") -def test_upload_bids_validation_ignore( - mocker: MockerFixture, bids_dandiset: SampleDandiset -) -> None: +def test_upload_bids(mocker: MockerFixture, bids_dandiset: SampleDandiset) -> None: iter_upload_spy = mocker.spy(LocalFileAsset, "iter_upload") - bids_dandiset.upload(existing="forced", validation="ignore") + bids_dandiset.upload(existing="forced") # Check whether upload was run iter_upload_spy.assert_called() # Check existence of assets: diff --git a/dandi/upload.py b/dandi/upload.py index f79369067..f0e718760 100644 --- a/dandi/upload.py +++ b/dandi/upload.py @@ -443,7 +443,8 @@ def _bids_discover_and_validate( else: bids_datasets_to_validate = bids_datasets bids_datasets_to_validate.sort() - validated_datasets = [] + valid_datasets: List[Path] = [] + invalid_datasets: List[Path] = [] for bd in bids_datasets_to_validate: validator_result = validate_bids(bd) valid = is_valid( @@ -451,8 +452,16 @@ def _bids_discover_and_validate( allow_missing_files=validation == "ignore", allow_invalid_filenames=validation == "ignore", ) - if valid: - validated_datasets.append(bd) - return validated_datasets + (valid_datasets if valid else invalid_datasets).append(bd) + if invalid_datasets: + raise RuntimeError( + f"Found {pluralize(len(invalid_datasets), 'BIDS dataset')}, which did not " + "pass validation:\n * " + + "\n * ".join([str(i) for i in invalid_datasets]) + + "\nTo resolve " + "this, perform the required changes or set the validation parameter to " + '"skip" or "ignore".' + ) + return valid_datasets else: return bids_datasets