Skip to content

Conversation

@sfc-gh-pczajka
Copy link
Contributor

Add python to test validator tests

Copilot AI review requested due to automatic review settings December 3, 2025 12:54
@sfc-gh-pczajka sfc-gh-pczajka requested a review from a team as a code owner December 3, 2025 12:54
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the test validator to support Python tests by adding Python test creation capabilities, directory setup, and feature file tagging alongside the existing Rust, JDBC, and ODBC test support.

Key changes:

  • Added Python test creation helper method and directory structure setup
  • Updated feature tags to include @python_e2e markers
  • Incremented test validation count from 3 to 4 languages

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# And I should have access to the system
result = conn.cursor().execute(f"SELECT * FROM temporary_resource")
assert len(result) is not None, "Should have system access"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Using is not None with len() is incorrect. The len() function returns an integer (0 or more), not None. This assertion will always pass because any integer (including 0) is not None. Consider using len(result) > 0 or assert result to check if the result is non-empty.

Suggested change
assert len(result) is not None, "Should have system access"
assert len(result) > 0, "Should have system access"

Copilot uses AI. Check for mistakes.
finally:
try:
cursor.execute(f"DROP TABLE IF EXISTS temporary_resource")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The variable cursor is referenced but not defined in the finally block. It should be conn.cursor() or cursor should be defined earlier in the test (e.g., cursor = conn.cursor() after line 1114). This will cause a NameError at runtime.

Copilot uses AI. Check for mistakes.

fn create_complete_python_login_test() -> &'static str {
r#"
def test_successful_login_with_valid_credentials(self, connect):
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The function signature includes self parameter, suggesting this is a class method. However, looking at existing Python e2e tests in the repository (e.g., test_put_get_basic_operations.py), standalone test functions are typically used without the self parameter. For class-based tests (like in test_pat.py), the test should be part of a proper test class. Consider either:

  1. Removing self if this is meant to be a standalone function test
  2. Wrapping this in a proper test class if it needs to be a method
Suggested change
def test_successful_login_with_valid_credentials(self, connect):
def test_successful_login_with_valid_credentials(connect):

Copilot uses AI. Check for mistakes.
def test_successful_login_with_valid_credentials(self, connect):
# Given I have valid credentials
credentials = setup_valid_credentials()
table_name = "test_login_table"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The variable table_name is defined but never used in the test. Consider removing it or using it in the test logic.

Suggested change
table_name = "test_login_table"

Copilot uses AI. Check for mistakes.
@sfc-gh-pczajka sfc-gh-pczajka force-pushed the pczajka-add-python-to-test-validator-tests branch from d6a9e55 to 03e4368 Compare December 3, 2025 12:58
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