Skip to content

CHORE: Deduplicate tests while preserving coverage and edge cases - #833

Merged
Jahnvi Thakkar (jahnvi480) merged 5 commits into
mainfrom
jahnvi/scaling-spoon
Oct 6, 2026
Merged

Jahnvi Thakkar (jahnvi480) merged 5 commits into
mainfrom
jahnvi/scaling-spoon

Conversation

@jahnvi480

@jahnvi480 Jahnvi Thakkar (jahnvi480) commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

AB#49061

Not applicable; the ADO work item above tracks this change.


Summary

  • Remove 29 overwritten test definitions and consolidate equivalent repeated scenarios without changing the surviving cursor/encoding test bodies.
  • Consolidate Binary UTF-8 tests while retaining all 104 distinct string inputs and 7 raw-byte inputs, and explicitly test rejection of surrogate code points. Remove checks of Python's decoder that misleadingly claimed native conversion coverage.
  • Preserve distinct database paths, NULL/LOB/encoding cases, and parameterized coverage; add a regression guard against same-scope duplicate test names.
  • Change tests only: production code, coverage exclusions, pipeline configuration, and reporting configuration are unchanged.

Coverage: The authoritative combined Python/C++ reports retain exactly the same covered-line sets across all 44 production files: 9,419 / 11,068 lines (85.10%), with zero previously covered lines lost. The existing report provides line coverage, not branch coverage.

Validation: Windows native extension build passed; 326 targeted unit tests and 254 retained integration tests passed. python -m black --check --line-length=100 mssql_python tests passed (101 files). Exact comparison confirmed all original Binary string/byte inputs are retained once. Selected-suite collection changed from 1,607 to 1,559 items.

Cross-platform CI: Baseline run 180444 tested 1cf04a1317b07b861f3b42f4147404006b5953ba; cleanup run 180469 tested 69e9c49cef5c9490b90d121555dd2e83869d40d8. All 17 jobs passed after retries. The cleanup run used the unchanged PR-validation pipeline on the feature branch.

OS / configuration Before After Change
Windows x64 - SQL 2022 5:32 4:29 -19.0%
Windows x64 - SQL 2025 7:08 7:04 -1.0%
Windows x64 - LocalDB 4:41 4:13 -9.9%
macOS - SQL 2022 (retry) 11:27 10:05 -11.9%
macOS - SQL 2025 (retry) 17:56 14:21 -20.0%
Ubuntu x64 - SQL 2022 3:14 2:43 -15.7%
Ubuntu x64 - SQL 2025 4:18 3:38 -15.7%
Debian x64 - SQL 2022 3:05 2:40 -13.6%
Debian x64 - SQL 2025 4:05 3:32 -13.5%
RHEL x64 3:15 2:39 -18.2%
Alpine x64 3:19 2:44 -17.3%
Ubuntu ARM64 13:57 13:59 +0.2%
Debian ARM64 18:00 17:13 -4.4%
RHEL ARM64 15:56 15:48 -0.8%
Alpine ARM64 (retry) 23:37 22:41 -4.0%

Times are successful pytest-step wall times, including coverage and command/container overhead but excluding queueing, dependency installation, builds, and failed-attempt costs. Both macOS jobs initially timed out during SQL setup before pytest; Alpine ARM64 initially failed the unchanged pooling-speed threshold. Those three jobs passed on attempt 2 without code changes. These are single-run observations with hosted-runner variability, not a demonstrated repeatable causal speedup.

Remove shadowed definitions and repeated scenarios, consolidate all unique Binary inputs, and guard against duplicate test names.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:50
@github-actions github-actions Bot added the pr-size: large Substantial code update label Oct 5, 2026
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

PR Performance Report

✅ No regression detected

No consistent slowdowns detected across all 2 environments.

0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS

Coverage: 2 of 2 environments completed. Advisory result; does not block merging.

Performance diagnostics

Phase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed.

No affected phases or call-count changes were recorded.

All database tasks and timings

Unix / SQL Server 2022

Database task Before After Paired change Result
Connection opening 10.567 ms 10.677 ms -1.2% no signal
SELECT queries 1.095 ms 1.087 ms +0.8% no signal
Row insertion 35.301 ms 34.980 ms +0.1% no signal
Executemany inserts 156.553 ms 158.787 ms +0.4% no signal
Fetch-all queries 122.651 ms 122.038 ms -0.5% no signal
Row-by-row fetching 14.116 ms 14.599 ms +1.9% no signal
Batched row fetching 121.344 ms 117.190 ms -1.9% no signal
Transaction commit and rollback 118.289 ms 116.358 ms -1.0% no signal
Arrow row fetching 95.799 ms 95.593 ms -0.2% no signal
100,000-row insertion 441.653 ms 451.937 ms +0.1% no signal
Row fetching in batches of 100 122.727 ms 122.410 ms +0.6% no signal
Row fetching in batches of 10,000 135.773 ms 138.765 ms +0.9% no signal
Repeated positional queries 34.100 ms 34.375 ms +1.5% no signal
Repeated named-parameter queries 36.872 ms 36.837 ms +0.1% no signal
Legacy 100,000-row insertion 353.476 ms 356.769 ms +1.0% no signal
Insertion with explicit input sizes 502.133 ms 494.410 ms -2.7% no signal
Joined aggregation queries 179.443 ms 177.388 ms -1.2% no signal
Large joined-result fetching 192.315 ms 189.296 ms +2.4% no signal
1.2-million-row fetching 3530.148 ms 3471.510 ms -0.7% no signal
Common table expression queries 5.472 ms 5.509 ms +2.4% no signal
256 KiB VARCHAR(MAX) / fetchall() 1.345 ms 1.321 ms +6.6% no signal
10,000 scalar values / fetchval() (debug disabled) 107.297 ms 106.960 ms +0.3% no signal

Unix / SQL Server 2025

Database task Before After Paired change Result
Connection opening 99.343 ms 97.487 ms -2.9% no signal
SELECT queries 1.192 ms 1.089 ms -6.8% no signal
Row insertion 36.027 ms 33.982 ms -5.7% no signal
Executemany inserts 155.384 ms 156.106 ms -0.4% no signal
Fetch-all queries 127.381 ms 124.489 ms -0.6% no signal
Row-by-row fetching 14.247 ms 14.310 ms +0.1% no signal
Batched row fetching 119.379 ms 120.328 ms +1.3% no signal
Transaction commit and rollback 118.426 ms 114.973 ms -1.4% no signal
Arrow row fetching 96.445 ms 94.499 ms -2.0% no signal
100,000-row insertion 469.381 ms 448.026 ms -3.0% no signal
Row fetching in batches of 100 121.960 ms 123.059 ms +1.9% no signal
Row fetching in batches of 10,000 136.178 ms 136.845 ms -0.7% no signal
Repeated positional queries 33.629 ms 33.674 ms -0.8% no signal
Repeated named-parameter queries 38.804 ms 36.237 ms -1.8% no signal
Legacy 100,000-row insertion 375.193 ms 380.107 ms +0.5% no signal
Insertion with explicit input sizes 486.823 ms 510.615 ms +3.0% no signal
Joined aggregation queries 160.254 ms 163.556 ms +2.1% no signal
Large joined-result fetching 179.601 ms 193.693 ms +1.2% no signal
1.2-million-row fetching 3602.054 ms 3538.029 ms -1.8% no signal
Common table expression queries 5.213 ms 5.367 ms +3.2% no signal
256 KiB VARCHAR(MAX) / fetchall() 1.542 ms 1.436 ms +0.8% no signal
10,000 scalar values / fetchval() (debug disabled) 119.085 ms 111.734 ms -9.1% no signal
Build and measurement details

ADO build 180764

PR head: ef9bc0fe19601be3ced66196dda1124721e342a9
Base: 4c4195d4bb57c947cdab76334175bac4e023a828
Measured merge: 9cb98fbb5c9009076e84b71fb1ec5e91b1830f5b

  • Unix / SQL Server 2022: Python 3.12.3, x86_64, SQL 16.0.4295.3; 5 paired comparisons and 1 warmup.
  • Unix / SQL Server 2025: Python 3.12.3, x86_64, SQL 17.0.5005.3; 5 paired comparisons and 1 warmup.

A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent.

The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes.

Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency.

Raw samples and logs are attached to the ADO run as profiler-* artifacts.

Comment thread tests/test_010_connection_string_parser.py Fixed
@jahnvi480
Jahnvi Thakkar (jahnvi480) marked this pull request as ready for review October 5, 2026 10:51

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The duplicate-name guard misses tests inside same-scope control-flow blocks, allowing silent overwrites.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Deduplicates test coverage while preserving distinct scenarios and edge cases.

Changes:

  • Removes redundant and overwritten tests.
  • Consolidates Binary UTF-8 cases and adds surrogate rejection coverage.
  • Adds an AST-based duplicate test-name guard.
File Description
tests/​test_test_definitions.py Adds duplicate-name detection.
tests/​test_015_pyformat_parameters.py Removes redundant parameter tests.
tests/​test_014_ddbc_bindings_coverage.py Deletes misleading conversion tests.
tests/​test_013_encoding_decoding.py Removes an overwritten EUC-KR test.
tests/​test_012_connection_string_integration.py Removes parser scenarios covered elsewhere.
tests/​test_010_connection_string_parser.py Parameterizes unknown-keyword cases.
tests/​test_008_auth.py Removes duplicate MSI coverage.
tests/​test_003_connection.py Removes duplicate cursor cleanup coverage.
tests/​test_002_types.py Consolidates Binary encoding edge cases.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/test_test_definitions.py Outdated
Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:53

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The new duplicate-name guard misses duplicate Test* classes, which can still silently replace collected tests.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)

Comment thread tests/test_test_definitions.py Outdated
Keep nested namespaces separate and cover module/class control-flow cases. Document the parser-only localhost scanner suppression without changing test inputs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 10:58

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The duplicate guard misses overwritten test classes, and a distinct EUC-KR VARCHAR path is removed.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Korean VARCHAR SQL_CHAR encoding regression coverage was removed

tests/​test_013_encoding_decoding.py:3314

This removal drops the only case combining Korean str values with an EUC-KR SQL_CHAR configuration and a VARCHAR column. The retained SQL_CHAR/EUC-KR case at lines 2465-2603 only sends ASCII, while the Korean round-trip at lines 6840-6886 uses NVARCHAR/SQL_WCHAR; therefore the PR's stated preservation of distinct encoding/database paths is not met. Retain a Korean VARCHAR/SQL_CHAR execution case, ideally with a meaningful result assertion.

Comment thread tests/test_test_definitions.py Outdated
Detect Test-prefixed class redefinitions within module and class control flow. Restore the shadowed Korean SQL_CHAR/EUC-KR scenario with isolated connection state and byte/Unicode storage assertions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 5, 2026 11:47

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The duplicate-name guard misses collected unittest.TestCase classes not named with the Test* prefix.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread tests/test_test_definitions.py
@github-actions

github-actions Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

85%


📈 Total Lines Covered: 9445 out of 11094
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection_pool.cpp: 82.3%
mssql_python.pybind.connection.connection.cpp: 83.1%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%
mssql_python.cursor.py: 92.5%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the cleanup broadly addresses the ask, but the newly enabled test breaks the supported rhel runs. requesting changes to resolve that failure and close the remaining duplicate-class gap raised in the existing thread.

Comment thread tests/test_013_encoding_decoding.py
Recognize directly inherited unittest TestCase classes and cover scope isolation. Install glibc-gconv-extra in both RHEL CI jobs so ODBC can convert Korean CP949 results without empty batch data; keep all fidelity assertions unchanged.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 6, 2026 07:43

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The PR description materially contradicts the included pipeline and environment configuration changes.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (1)

Comment thread eng/pipelines/pr-validation-pipeline.yml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the cleanup preserves the existing cases, and the follow-up resolves both review findings without weakening the assertions. approving.

@jahnvi480
Jahnvi Thakkar (jahnvi480) merged commit 666f3cb into main Oct 6, 2026
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: large Substantial code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants