fix: correct +09:00 timezone integration test assertion#329
Merged
Conversation
The MySQL integration test for the new `timezone` source option (#321) asserted the wrong UTC instant. KST is UTC+9, so the naive DATETIME `2025-09-29 02:31:23` interpreted as `+09:00` is `2025-09-28T17:31:23Z` (the previous day), not `2025-09-29T17:31:23Z`. mysql2's parseDateTime does `new Date(`${str}${timezone}`)`, which produces the previous-day instant — so the assertion would have failed against a real container, indicating the integration suite was not run before merge. Fix the expected value and the explanatory comment. Also drop the redundant `typeof source.timezone !== "string"` guard in the TOML timezone validation: the field is already narrowed to a string, and RegExp.test() coerces safely, so the guard can never be the sole cause of a throw (unlike search_path's guard, which is load-bearing for its .trim() call). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR corrects the expected UTC instant in the MySQL timezone integration test for timezone = "+09:00" and simplifies TOML timezone validation logic.
Changes:
- Fixes the MySQL integration test assertion to expect the correct previous-day UTC timestamp for
+09:00. - Updates the test comment to accurately explain the date rollover when converting KST → UTC.
- Removes the
typeof source.timezone !== "string"check from TOML timezone validation (note: this introduces a validation hole; see review comment).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/connectors/tests/mysql.integration.test.ts | Updates the +09:00 DATETIME→UTC expectation and clarifies the reasoning in comments. |
| src/config/toml-loader.ts | Simplifies timezone validation condition for MySQL/MariaDB sources. |
Removing the typeof check opened a validation hole flagged in review: a single-element TOML array such as timezone = ["local"] coerces via RegExp.test() to the passing string "local", so the array would reach the driver as a non-string. Restore the typeof guard and add a regression test for the array case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+460
to
466
| // Accepted by mysql2/mariadb drivers: "local", "Z", or "±HH:MM" (e.g., "+09:00"). | ||
| // The typeof guard is required: TOML can yield non-strings (e.g. arrays), and | ||
| // RegExp.test() would coerce a single-element array like ["local"] to a passing | ||
| // string before it reaches the driver as a non-string value. | ||
| if ( | ||
| typeof source.timezone !== "string" || | ||
| !/^(?:local|Z|[+-]\d\d:\d\d)$/.test(source.timezone) |
Member
Author
There was a problem hiding this comment.
Correct — the guard was dropped then restored after the earlier comment, so the net validation diff is now zero (only an explanatory comment + regression test remain). Updated the PR title and description to match: the change set is now the +09:00 test-assertion fix plus the array-case hardening, with no guard removal.
tianzhou
changed the title
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #321 (
timezonesource option for MySQL/MariaDB). The main fix is a wrong test assertion; the validation code is left functionally unchanged.The bug
The MySQL integration test asserted the wrong UTC instant. KST is UTC+9, so the naive DATETIME
2025-09-29 02:31:23interpreted as+09:00is2025-09-28T17:31:23Z(the previous day), not2025-09-29T17:31:23Z.mysql2's
parseDateTimedoesnew Date(\${str}${timezone}`)`, which yields the previous-day instant:Because the old expected value is never producible by the driver, the test would have failed against a real container — indicating the Testcontainers integration suite was not run before #321 merged. The same off-by-one-day miscalculation appears in #321's description table (immutable history, nothing to fix there). The
"Z"test was already correct and is unchanged.Validation hardening
While here, the timezone validation in
toml-loader.tsis left intact but gains:typeof source.timezone !== "string"guard is required — TOML can yield non-strings, andRegExp.test()coerces a single-element array like["local"]to a passing string, which would otherwise reach the driver as a non-string.["local"]array case.(An earlier revision of this PR briefly dropped the guard; review correctly flagged the resulting hole, so it was restored. The net diff to the validation logic is therefore zero — only the comment and the new test are added.)
Testing
pnpm test src/config/__tests__/toml-loader.test.ts— 109 pass, including the new array-rejection case and the existing invalid-format (Asia/Seoul) case.+09:00assertion was verified against mysql2's exactparseDateTimelogic. The full integration test still requires Docker to run.🤖 Generated with Claude Code