bito-code-review[bot] commented on code in PR #43253:
URL: https://github.com/apache/superset/pull/43253#discussion_r3820447220
##########
tests/integration_tests/utils/machine_auth_tests.py:
##########
@@ -49,6 +49,27 @@ def test_authenticate_browser_context_sets_cookies(self):
c["name"] == "session" and c["value"] == "abc123" for c in
cookies_added
)
+ def
test_authenticate_browser_context_sets_cookies_after_navigation_timeout(self):
+ """Navigation timeouts do not prevent auth cookies from being set."""
+ user = self.get_user("admin")
+ provider = machine_auth_provider_factory.instance
+
+ mock_context = MagicMock()
+ mock_page = MagicMock()
+ mock_page.goto.side_effect = PlaywrightTimeout("Navigation timed out")
+ mock_context.new_page.return_value = mock_page
+
+ with patch.object(provider, "get_cookies", return_value={"session":
"abc123"}):
+ result = provider.authenticate_browser_context(mock_context, user)
+
+ assert result is mock_context
+ mock_page.goto.assert_called_once()
+ mock_context.add_cookies.assert_called_once()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing clear_cookies assertion</b></div>
<div id="fix">
The new test verifies cookies are added after a `PlaywrightTimeout`, but
does not verify `clear_cookies()` is still called — the original test at line
45 includes this assertion. The implementation at `machine_auth.py:87` always
calls `browser_context.clear_cookies()` after the try/except block, so this
assertion should be present to catch any future regression where the timeout
path incorrectly skips clearing.
</div>
</div>
<small><i>Code Review Run #7e2347</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
