FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix `test_closing_connection_closes_commands` by varun-edachali-dbx · Pull Request #584 · databricks/databricks-sql-python · GitHub

/ databricks-sql-python Public

fix test_closing_connection_closes_commands - #584

Merged
varun-edachali-dbx merged 7 commits into
mainfrom
test-closes-commands
Jun 10, 2025
ConversationCommits (7)ChecksFiles changed
Merged

fix test_closing_connection_closes_commands#584
varun-edachali-dbx merged 7 commits into
mainfrom
test-closes-commands

Conversation

varun-edachali-dbx commented Jun 4, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • Bug Fix

Description

Currently, test_closing_connection_closes_commands is incorrect because:

  • it iterates over values of closed but does not use them distinctly in the subTests. The intention is likely to set them as the initial state of has_been_closed_server_side, based on the comment.
  • the entire result set is Mocked, so when we check the final value of has_been_closed_server_side, it may extract an attribute that does not exist. This assertion will pass even if we do not set has_been_closed_server_side to True in our close() in the ResultSet. This is because the attribute is created as a new mock on being accessed, and mocks are by-default truthy. In order to test this, even if we change the assertion to self.assertTrue(mock_result_set_class.return_value.random_attr), the test will pass, despite random_attr not existing on the type.

How is this tested?

  • Unit tests
  • E2E Tests
  • Manually
  • N/A

Related Tickets & Documents

N/A

[init] working code for corrected test
47f4547 
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
excess comments
2f666fb 
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>

jayantsing-db commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

Hi @jprakash-db, could you please take a look as this fix is intended for the main branch.

jprakash-db reviewed Jun 9, 2025
View reviewed changes
Comment thread tests/unit/test_client.py
Comment thread tests/unit/test_client.py
Comment thread tests/unit/test_client.py Outdated
Comment thread tests/unit/test_client.py Outdated
use pytest parametrize instead of unittest subtests, switch to pytest…
f96936a 
… instead of unittest primitives

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
initialisations, function calls, then assertions
5521f21 
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
remove pytest parametrize, move back to unittest subTest
c22a53f 
to allow keeping the test inside ClientTestSuite

Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
clean up docstring
c3d8b0f 
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
remove pytest import
dea05e6 
Signed-off-by: varun-edachali-dbx <varun.edachali@databricks.com>
jprakash-db approved these changes Jun 10, 2025
View reviewed changes

jprakash-db left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Thanks for making the changes

varun-edachali-dbx merged commit 3842583 into main Jun 10, 2025
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jayantsing-db jayantsing-db approved these changes
jprakash-db jprakash-db approved these changes
deeksha-db Awaiting requested review from deeksha-db
samikshya-db Awaiting requested review from samikshya-db
jackyhu-db Awaiting requested review from jackyhu-db
madhav-db Awaiting requested review from madhav-db
gopalldb Awaiting requested review from gopalldb
vikrantpuppala Awaiting requested review from vikrantpuppala
shivam2680 Awaiting requested review from shivam2680

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL