[MAINTENANCE] Assert store access in DatasourceDict just-in-time tests#11949
Open
anxkhn wants to merge 1 commit into
Open
[MAINTENANCE] Assert store access in DatasourceDict just-in-time tests#11949anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
The two 'requests_store_just_in_time' tests set store.list_keys_count = 0/1 instead of asserting on it, so they asserted nothing and passed even when DatasourceDict.data never touched the store. The intended counter was also wrong: .data calls DatasourceStore.get_all(), not list_keys(), which the spy did not track (list_keys_count stays 0). Track get_all() in DatasourceStoreSpy and assert store.get_all_count goes from 0 to 1, turning both tests into real regression guards for just-in-time store access.
👷 Deploy request for niobium-lead-7998 pending review.Visit the deploys page to approve it
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two unit tests in
tests/datasource/test_datasource_dict.pythat are meant to verifyDatasourceDictreads its store just-in-time were not asserting anything, so they passed unconditionally and gave no regression protection.test_datasource_dict_data_property_requests_store_just_in_timetest_datasource_dict___contains___requests_store_just_in_timeBoth wrote:
Lines 100/102 (and 111/113 in the second test) are plain assignments to the spy counter, not assertions, so nothing is ever checked. Every other test in the same file uses the
assert store.<counter> == Npattern (for exampleassert store.set_count == 1).Root cause
Two problems compound:
=was written whereassert ... ==was intended, so the tests assert nothing.DatasourceDict.data(great_expectations/datasource/datasource_dict.py:71) callsself._datasource_store.get_all(), notlist_keys().DatasourceStoreSpytracksget/set/list_keys/has_key/remove_keybut does not overrideget_all, solist_keys_countstays0on a.dataaccess. Even the naive repair (assert store.list_keys_count == 1) would fail.The behavior these tests are named for is documented at
great_expectations/datasource/datasource_dict.py:63-67:Fix
Have the spy track
get_all()and assert on it, so both tests become real red -> green guards for just-in-time store access:get_all_counttoDatasourceStoreSpy(initialized in__init__and reset alongside the existing counters) and overrideget_all()to increment it viasuper().get_all(), matching the file's existing spy-override style.assert store.get_all_count == 0before the.dataaccess andassert store.get_all_count == 1after.Testing
python -m pytest tests/datasource/test_datasource_dict.py -q-> 13 passed..dataskip the store makes both tests fail withassert 0 == 1; reverting restores green. Before this change they passed against that same breakage.invoke lint --path tests/datasource/test_datasource_dict.pypasses (import-sort,ruff format --check,ruff check).PR checklist (from the repo template)
-- see note below; no matching issue exists, and recent test-harness
[MAINTENANCE]PRs merged without one.invoke lint(usesruff format+ruff check)