Skip to content

Conversation

@sadpandajoe
Copy link
Member

@sadpandajoe sadpandajoe commented Nov 19, 2025

SUMMARY

Fixes intermittent test failure in tests/integration_tests/charts/commands_tests.py::TestChartsUpdateCommand::test_update_v1_response that occurred on MySQL due to datetime precision handling with the error:

FAILED tests/integration_tests/charts/commands_tests.py::TestChartsUpdateCommand::test_update_v1_response - 
assert datetime.datetime(2025, 11, 17, 18, 27, 41) != datetime.datetime(2025, 11, 17, 18, 27, 41) + 
where datetime.datetime(2025, 11, 17, 18, 27, 41) = Average and Sum Trends.last_saved_at

Root Cause:

  • MySQL's DATETIME type defaults to second-level precision, truncating microseconds from Python's datetime.now()
  • When the original test executed in under one second, timestamps appeared identical at second precision
  • Initial fix attempt added .replace(microsecond=0) for comparison, but failed when last_saved_at was None

Solution:
Split the original test into two focused, deterministic tests:

  1. test_update_sets_last_saved_at: Tests None → datetime transition
    - Explicitly sets last_saved_at = None before update
    - Verifies field gets populated after update
    - No sleep needed
  2. test_update_changes_last_saved_at: Tests datetime → datetime transition
    - Seeds last_saved_at = datetime.now() before update
    - Sleeps 1 second to guarantee crossing second boundary for MySQL
    - Uses .replace(microsecond=0) for database-agnostic comparison
    - Verifies timestamp actually changed

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@bito-code-review
Copy link
Contributor

bito-code-review bot commented Nov 19, 2025

Code Review Agent Run #38e96e

Actionable Suggestions - 0
Additional Suggestions - 2
  • tests/integration_tests/charts/commands_tests.py - 2
    • Assert statement used in test code · Line 393-393
      Using `assert` statements in test code is flagged by static analysis. Consider using `self.assertEqual()` or similar test framework assertions for better error reporting. Similar issues exist on lines 393 and 396.
      Code suggestion
       @@ -392,5 +392,5 @@
      -        # Normalize microseconds for database-agnostic comparison
      -        assert chart.last_saved_at.replace(microsecond=0) != last_saved_before.replace(
      -            microsecond=0
      -        )
      -        assert chart.last_saved_by == user
      +        # Normalize microseconds for database-agnostic comparison
      +        self.assertNotEqual(chart.last_saved_at.replace(microsecond=0), last_saved_before.replace(
      +            microsecond=0,
      +        ))
      +        self.assertEqual(chart.last_saved_by, user)
    • Missing trailing comma in function call · Line 394-394
      Missing trailing comma in multi-line function call on line 394. Adding trailing commas improves code consistency and reduces diff noise in version control.
      Code suggestion
       @@ -393,3 +393,3 @@
      -        assert chart.last_saved_at.replace(microsecond=0) != last_saved_before.replace(
      -            microsecond=0
      -        )
      +        assert chart.last_saved_at.replace(microsecond=0) != last_saved_before.replace(
      +            microsecond=0,
      +        )
Review Details
  • Files reviewed - 1 · Commit Range: 9dc6dbb..9dc6dbb
    • tests/integration_tests/charts/commands_tests.py
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • MyPy (Static Code Analysis) - ✔︎ Successful
    • Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@dosubot dosubot bot added the data:connect:mysql Related to MySQL label Nov 19, 2025
Copilot finished reviewing on behalf of sadpandajoe November 19, 2025 01:04
Copy link
Contributor

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 fixes a flaky MySQL integration test in test_update_v1_response that intermittently fails due to MySQL's DATETIME type truncating microseconds to second-level precision.

Key changes:

  • Added time.sleep(1) to ensure timestamps differ at second-level precision
  • Modified timestamp comparison to normalize microseconds using .replace(microsecond=0) for database-agnostic behavior

@sadpandajoe sadpandajoe marked this pull request as draft November 19, 2025 18:21
@sadpandajoe sadpandajoe force-pushed the fix-flakey-test-mysql-integration branch from 9dc6dbb to 4b9bfae Compare November 20, 2025 06:46
@sadpandajoe sadpandajoe marked this pull request as ready for review November 20, 2025 18:05
Split into two focused tests to properly handle MySQL's DATETIME second-level
precision and the None → datetime transition:

1. test_update_sets_last_saved_at: Tests that last_saved_at gets set when
   previously None
2. test_update_changes_last_saved_at: Tests that last_saved_at changes when
   already set, using time.sleep(1) and .replace(microsecond=0) for MySQL
   precision handling

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@sadpandajoe sadpandajoe force-pushed the fix-flakey-test-mysql-integration branch from 4b9bfae to 4641b4e Compare November 20, 2025 19:04
@sadpandajoe sadpandajoe requested a review from Copilot November 20, 2025 20:02
Copilot finished reviewing on behalf of sadpandajoe November 20, 2025 20:04
Copy link
Contributor

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


chart_to_update = db.session.query(Slice).get(pk)
chart_to_update.last_saved_at = datetime.now()
db.session.commit()
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

[nitpick] After setting last_saved_at = datetime.now() and committing, the Python object may still contain microseconds that were truncated by MySQL during the database write. To ensure last_saved_before contains the actual database value (without microseconds), consider querying the chart fresh after the commit:

chart_to_update = db.session.query(Slice).get(pk)
chart_to_update.last_saved_at = datetime.now()
db.session.commit()
# Refresh to get the database value with truncated microseconds
chart_to_update = db.session.query(Slice).get(pk)
last_saved_before = chart_to_update.last_saved_at

This makes the test more explicit and avoids relying on ORM refresh behavior.

Suggested change
db.session.commit()
db.session.commit()
# Refresh to get the database value with truncated microseconds
chart_to_update = db.session.query(Slice).get(pk)

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

After committing chart_to_update.last_saved_at, the in-memory Python
object still contains microseconds, but MySQL's DATETIME(0) has
truncated them in the database. This caused the test to compare against
the wrong baseline value.

Adding db.session.refresh() after commit ensures we capture the actual
database value with MySQL's second-level precision before comparing.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copy link
Contributor

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

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

Thanks, this test was annoying!

@sadpandajoe sadpandajoe merged commit a0c29cc into master Nov 21, 2025
65 checks passed
@sadpandajoe sadpandajoe deleted the fix-flakey-test-mysql-integration branch November 21, 2025 21:29
LuisSanchez pushed a commit to LuisSanchez/superset that referenced this pull request Nov 25, 2025
kshi020302 pushed a commit to jl141/superset that referenced this pull request Nov 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants