Skip to content

Fix no-op assertion in MaxCompute run_sql test#69406

Open
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:fix/alibaba-maxcompute-noop-assertion
Open

Fix no-op assertion in MaxCompute run_sql test#69406
anxkhn wants to merge 1 commit into
apache:mainfrom
anxkhn:fix/alibaba-maxcompute-noop-assertion

Conversation

@anxkhn

@anxkhn anxkhn commented Jul 5, 2026

Copy link
Copy Markdown

The test_run_sql case in the Alibaba provider asserted
mock_client.run_sql.asssert_called_once_with(...) with a misspelled asssert
(three s's). On a MagicMock, that attribute is auto-created as a child mock, so
calling it returns a truthy mock and the wrapping assert always passed. This is
the only assertion of MaxComputeHook.run_sql's argument forwarding to
client.run_sql, so the forwarded keyword arguments were never actually checked,
including the branch that derives priority from the odps.instance.priority
hint when no explicit priority is given.

This replaces the no-op with a real assert_called_once_with (a bare statement,
matching the sibling test_get_instance / test_stop_instance_success cases) and
parametrizes the test over three cases so the forwarded arguments are genuinely
verified:

  • explicit-priority: an explicit priority is forwarded unchanged (derivation
    branch not taken).
  • priority-from-hints: priority=None with hints={"odps.instance.priority": 5}
    is derived to 5 (derivation branch taken, previously untested).
  • no-priority: priority=None and hints=None stays None.

Test-only change. Confirmed red -> green: correcting the assertion turns it from
always-passing into a real check, and disabling the derivation branch in the hook
makes only the priority-from-hints case fail. rg "asssert" across the repo now
returns zero matches.


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

The test asserted `mock_client.run_sql.asssert_called_once_with(...)` with a
misspelled `asssert`. On a MagicMock that attribute is auto-created, so calling
it returns a truthy mock and the wrapping `assert` always passed, leaving the
hook's argument forwarding to `client.run_sql` unverified, including the branch
that derives `priority` from the `odps.instance.priority` hint.

Use the real `assert_called_once_with` and parametrize the test to cover
explicit priority, priority derived from hints, and no priority so the forwarded
arguments are actually checked.

Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
@Vamsi-klu

Copy link
Copy Markdown
Contributor

Confirmed the root cause against the hook source. The old line was assert mock_client.run_sql.asssert_called_once_with(...) with a triple-s typo; on a MagicMock that attribute auto-creates a child mock, calling it returns a truthy mock, and the wrapping assert passed unconditionally, so the forwarded kwargs were never checked. The fix to a bare assert_called_once_with matches the sibling test_get_instance / test_stop_instance_success style. run_sql does if priority is None and hints is not None: priority = hints.get("odps.instance.priority"), then forwards priority=priority, hints=hints unchanged. The three parametrized cases line up: explicit priority=1 forwarded unchanged, (None, {"odps.instance.priority": 5}) -> 5, (None, None) -> None, and the hints dict is passed through unmodified so hints=hints is correct. The priority-from-hints branch was previously never exercised and now is. LGTM; nice catch on the silently-passing assertion.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants