sql driver tests#2824
Open
zachmu wants to merge 18 commits into
Open
Conversation
Contributor
|
Contributor
|
Ito Test Report ❌15 test cases ran. 3 failed, 12 passed. The unified run executed 15 test cases with 12 passing and 3 failing (plus one non-executed verification section with no runnable assertions), with credential/TLS behavior, concurrency regressions, strict unknown-cluster-field parsing, default config loading, skip gating, and error_match validation all behaving as expected. The most important findings were three medium, PR-introduced defects: cluster settings are accepted in config parsing but silently inactive at runtime with no unsupported-feature signal, retry_attempts do not cover restart_server (splitting a single logical assertion across attempts), and malformed query steps with neither query nor exec are incorrectly treated as successful no-ops. ❌ Failed (3)
🟠 Cluster config parses but runtime is silently inactive
Relevant code:
// ClusterConfig mirrors Dolt's cluster replication configuration. Cluster
// replication is not yet implemented in Doltgres; this is accepted so that
// cluster config files parse (the integration tests for cluster
// replication are ported but skipped until the feature lands). The shape
// mirrors Dolt's ClusterYAMLConfig.
ClusterCfg *DoltgresClusterConfig `yaml:"cluster,omitempty" minver:"TBD"`
}
// DoltgresClusterConfig mirrors Dolt's cluster replication configuration so
// that ported cluster config files parse under UnmarshalStrict.
func (cfg *DoltgresConfig) ClusterConfig() doltservercfg.ClusterConfig {
return nil
}
# The remotes API (used to serve/clone databases between servers) is not yet
# implemented in Doltgres, so this test is skipped. The config, remotesapi
# port wiring, and dolt_clone/dolt_commit flow are translated faithfully so it
# can be enabled once the feature lands.
skip: "cluster/remotesapi replication not yet implemented in Doltgres"🟠 Retry flow splits restart semantics
Relevant code:
if c.RetryAttempts > 1 {
RetryTestRun(t, c.RetryAttempts, func(t TestingT) {
db, err := server.DB(c)
require.NoError(t, err)
defer db.Close()
conn, err := db.Conn(context.Background())
require.NoError(t, err)
defer conn.Close()
for _, q := range c.Queries {
RunQueryAttempt(t, conn, q, &ports)
}
})
}
if c.RestartServer != nil {
args := c.RestartServer.Args
if args != nil {
tmplArgs := make([]string, len(*args))
for i := range tmplArgs {
tmplArgs[i] = ports.ApplyTemplate((*args)[i])
}
prepared := prepareDoltgresServerArgs(t, server.Cmd.Dir, server.Name, server.Port, tmplArgs)
args = &prepared
}
err := server.Restart(args, c.RestartServer.Envs)
require.NoError(t, err)
}
// Rarely needed, allows the entire connection assertion to be retried
// on an assertion failure. Use this is only for idempotent connection
// interactions and only if the sql-server is prone to tear down the
// connection based on things that are happening, such as cluster role
// transitions.
RetryAttempts int `yaml:"retry_attempts"`🟠 Malformed query step passes silently
Relevant code:
// The primary interaction of a |Connection|. Either |Query| or |Exec| should
// be set, not both.
type Query struct {
if q.Query != "" {
ctx, c := context.WithTimeout(context.Background(), timeout)
defer c()
rows, err := conn.QueryContext(ctx, q.Query, args...)
if err == nil {
defer rows.Close()
}
if q.ErrorMatch != "" {
require.Error(t, err, "expected error running query %s", q.Query)
require.Regexp(t, q.ErrorMatch, err.Error())
return
}
require.NoError(t, err)
// ... assertion path omitted ...
} else if q.Exec != "" {
ctx, c := context.WithTimeout(context.Background(), timeout)
defer c()
exec := q.Exec
exec = ports.ApplyTemplate(exec)
_, err := conn.ExecContext(ctx, exec, args...)
if q.ErrorMatch == "" {
require.NoError(t, err, "error running query %s: %v", q.Exec, err)
} else {
require.Error(t, err)
require.Regexp(t, q.ErrorMatch, err.Error())
}
}✅ Passed (12)
Commit: Tell us how we did: Give Ito Feedback |
|
Diff SummaryThis diff run surfaced Overall, 15 of 17 test cases passed, indicating strong suite health across cluster compatibility signaling, skip-gated coverage behavior, and amend-concurrency stability (including repeated and resource-constrained race runs). The two medium-severity failures were that malformed YAML query steps can pass silently without validation (risking hidden missing assertions) and concurrent CREATE/DROP database activity can leave residual sk7_* schemas, consistent with a real non-atomic schema update race. Tests run by Ito
Tests that are no longer relevantBelow are tests that previously ran and are no longer relevant:
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Concurrent database cleanup leaves residual schemas
Tip Reply with @itoqa to send us feedback on this test run. |
…ql into zachmu/sql-driver-tests
|
Diff SummaryThis diff run surfaced Coverage focused on core database behavior and safety boundaries, including read-only protections, cluster and replication gating, metrics authentication/config handling, startup readiness, and recovery after garbage-collection-related reconnects. The run exercised both normal workflows and edge-case or adversarial conditions around unsupported or misconfigured paths, with overall product behavior remaining stable for the areas touched by this change. Safe to merge — the issues observed are pre-existing and tied to unsupported or known edge-path behavior rather than regressions introduced by this PR. The changed code appears low risk because critical runtime flows and recovery paths remained healthy, with no new PR-attributable failures. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟠 Localhost metrics JWT enforcement bypass
Evidence Package🟠 Metrics JWKS verification is hard-disabled
Evidence Package🟠 Read-only CALL mutability mismatch
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
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
1 participant
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.
Ported tests from dolt via agent, manually pruned down the ones that aren't applicable or we think are adequately tested by Dolt itself.
Most skips are due to cluster replication not yet working.