Skip to content

test(dataset_tests): make tests self-cleaning and remove global-list assertion#117

Merged
zvika-tonic merged 1 commit into
mainfrom
fix/test-get-all-datasets-flakiness
Jun 17, 2026
Merged

test(dataset_tests): make tests self-cleaning and remove global-list assertion#117
zvika-tonic merged 1 commit into
mainfrom
fix/test-get-all-datasets-flakiness

Conversation

@zvika-tonic

Copy link
Copy Markdown
Contributor

The Python SDK CI matrix (python-version: [3.9, 3.13]) runs both jobs against the same shared PRAPP backend with the same org/API key. This caused cross-matrix flakes in tests/tests/dataset_tests/, most visibly test_get_all_datasets, which:

  1. Created two datasets without ever deleting them, leaking state into subsequent runs of the other matrix entry.
  2. Asserted len(get_all_datasets()) >= 2, which is a global-list assertion that depends on backend state outside the test's control and can also race with the SDK's internal N+1 list -> get_dataset_by_name loop (a parallel deletion can surface as a 404 mid-loop).

This change:

  • Adds tests/tests/dataset_tests/conftest.py with a function-scoped 'created_datasets' fixture: tests append each created dataset name to the yielded list, and the fixture deletes every registered name on teardown (best-effort per name, so a single failure does not mask others). This mirrors the create -> yield -> delete pattern already used by setup_dataset in tests/conftest.py.
  • Drops the global len(ds_all) >= 2 assertion in test_get_all_datasets; it now only checks that the two UUID-named datasets it created appear in the result, which is what the test actually intends to verify.
  • Wires the new fixture into every backend-touching test in dataset_tests/ that previously created datasets without teardown: test_dataset_lists, test_edit_dataset (all four tests, including the rename case where both pre- and post-rename names are registered), test_get_dataset (both tests), test_get_dataset_files (both tests), test_pii_occurrences (both tests), test_upload_files_to_dataset (all three tests). test_entity_mappings is unaffected (pure unit tests with a mocked client).

Out of scope (tracked separately): the underlying N+1 in tonic_textual/services/dataset.py::get_all_datasets, which is still race-prone against concurrent deletions even with this fix.

…assertion

The Python SDK CI matrix (python-version: [3.9, 3.13]) runs both jobs
against the same shared PRAPP backend with the same org/API key. This
caused cross-matrix flakes in tests/tests/dataset_tests/, most visibly
test_get_all_datasets, which:

  1. Created two datasets without ever deleting them, leaking state into
     subsequent runs of the other matrix entry.
  2. Asserted len(get_all_datasets()) >= 2, which is a global-list
     assertion that depends on backend state outside the test's control
     and can also race with the SDK's internal N+1
     list -> get_dataset_by_name loop (a parallel deletion can surface
     as a 404 mid-loop).

This change:

- Adds tests/tests/dataset_tests/conftest.py with a function-scoped
  'created_datasets' fixture: tests append each created dataset name to
  the yielded list, and the fixture deletes every registered name on
  teardown (best-effort per name, so a single failure does not mask
  others). This mirrors the create -> yield -> delete pattern already
  used by setup_dataset in tests/conftest.py.
- Drops the global len(ds_all) >= 2 assertion in test_get_all_datasets;
  it now only checks that the two UUID-named datasets it created appear
  in the result, which is what the test actually intends to verify.
- Wires the new fixture into every backend-touching test in
  dataset_tests/ that previously created datasets without teardown:
  test_dataset_lists, test_edit_dataset (all four tests, including the
  rename case where both pre- and post-rename names are registered),
  test_get_dataset (both tests), test_get_dataset_files (both tests),
  test_pii_occurrences (both tests), test_upload_files_to_dataset (all
  three tests). test_entity_mappings is unaffected (pure unit tests
  with a mocked client).

Out of scope (tracked separately): the underlying N+1 in
tonic_textual/services/dataset.py::get_all_datasets, which is still
race-prone against concurrent deletions even with this fix.
@zvika-tonic zvika-tonic enabled auto-merge (squash) June 16, 2026 21:30
@zvika-tonic zvika-tonic merged commit 832b520 into main Jun 17, 2026
3 checks passed
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.

2 participants