Skip to content

Conversation

@zihuaihuai
Copy link

Fix Test Logic Bug in Participant Filtering Test

Summary

This PR fixes a bug in the test test_participant_label_doesnt_filter_comps_when_subject_has_filter_no_wcard where the test was creating a modified config but not using it in the actual test call, causing intermittent test failures.

Problem

The test was experiencing flaky behavior - sometimes passing, sometimes failing. Investigation revealed that the test was:

  1. Creating a config from the dataset using create_snakebids_config(rooted)
  2. Modifying this config by adding subject filters to each component
  3. But then calling generate_inputs() with the original create_snakebids_config(rooted) instead of the modified config

This meant the subject filters were never actually applied during testing, making the test's behavior non-deterministic and dependent on the randomly generated test data from Hypothesis.

Solution

Fixed the test to use the modified config variable in the generate_inputs() call instead of recreating the config. This ensures that the subject filters are properly applied during the test, making the test behavior consistent and deterministic.

Changes

  • Modified test_participant_label_doesnt_filter_comps_when_subject_has_filter_no_wcard in snakebids/tests/test_generate_inputs.py
  • Changed the generate_inputs() call to use the modified config instead of create_snakebids_config(rooted)

Testing

  • The previously flaky test now passes consistently
  • All existing tests continue to pass
  • Pre-commit hooks (ruff, pyright, codespell) all pass

Impact

This fix ensures reliable test execution and eliminates a source of CI flakiness. The test now properly validates the intended behavior: that participant label filtering doesn't affect components when subject entities have explicit filters applied.

@github-actions github-actions bot requested a review from akhanf August 27, 2025 16:19
@github-actions github-actions bot added the bug Something isn't working label Aug 27, 2025
@pvandyken
Copy link
Contributor

pvandyken commented Nov 5, 2025

Thanks for bringing this up. I actually don't think your fix will do anything for flaky test behaviour... that's more likely because this test is breaking the hypothesis time limits (these examples where an entire dataset generated by hypothesis is created and indexed need to be phased out eventually).

But there is an issue here, the test is pretty clearly incorrect and ineffectual. And I think there's some underlying issues with the query code. I'm going to close this I'm going to create a new issue regarding issues with the query code.

I'm going to rework this test a bit more before merging. It really should be testing that no filtering occurs when there is no subject wildcard. We already have a test for when subject has a filter.

@pvandyken pvandyken closed this Nov 5, 2025
@pvandyken pvandyken reopened this Nov 5, 2025
zihuaihuai and others added 5 commits November 7, 2025 14:47
- Fix test_participant_label_doesnt_filter_comps_when_subject_has_filter_no_wcard
- Test was creating modified config but not using it in generate_inputs call
- This caused intermittent test failures due to non-deterministic behavior
- Now uses the modified config consistently for reliable test execution
Previously returned a regex with a negative look-ahead on an empty
string. This would match and exclude everything.
Previously tested only by integration type tests in
test_generate_inputs.
Tests involve pybids indexing, and previously used hypothesis to
generate examples. Remove hypothesis from all cases and parametrize with
focused cases. Test logic between generate_inputs and get_matching_files
with patches
@pvandyken pvandyken force-pushed the fix/participant-filtering-test-config-bug branch from a0737dc to 23d4676 Compare November 13, 2025 23:13
@pvandyken pvandyken changed the title Fix/participant filtering test config bug Update _querying and participant-label filtering Nov 13, 2025
@pvandyken
Copy link
Contributor

I've used this PR now to add a new unit test suite for the _querying.py module. The affected test class in test_generate_inputs.py has been greatly pared down, and hypothesis has been removed.

@akhanf, in the past, I've overused hypothesis on long-running tests (e.g. that run pybids for indexing). It's resulted in a lot of long-running, flaky tests. This fix represents a move toward a fix: use hypothesis in short-running unit tests, test logic in higher-order functions with mocker, and test integration with focused examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants