postgresql-interfaces/psqlodbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing
2+ messages / 2 participants
[nested] [flat]
* [postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing
@ 2026-04-20 04:09 "jarvis24young (@jarvis24young)" <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: jarvis24young (@jarvis24young) @ 2026-04-20 04:09 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
Adds test/src/bracket-parse-test.c (20 test cases covering all branches of the OPENING_BRACKET switch in dconn_get_attributes) and a Chinese analysis document at test/docs/bracket-parse-analysis.md describing the parser's control flow, risk points, and how to run the tests (including ASan/UBSan setups for stability hardening).
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing
@ 2026-04-23 10:55 "davecramer (@davecramer)" <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: davecramer (@davecramer) @ 2026-04-23 10:55 UTC (permalink / raw)
To: postgresql-interfaces/psqlodbc <[email protected]>
1. Incomplete PR — missing integration into the test suite
- No test/tests modification (the test isn't added to TESTBINS)
- No test/expected/bracket-parse.out file
- Without these, make installcheck won't run this test. The analysis doc describes how to add
them but the PR doesn't actually do it. This should be included in the PR itself.
2. Documentation is entirely in Chinese
- The psqlodbc project is English-language. Both markdown files should be in English, or at
minimum have English translations. This is a barrier for other contributors and maintainers.
3. `test/docs/` is a new directory with no precedent
- The project has no existing test/docs/ directory. Analysis documents and coverage reports don't
belong in the source tree — they're ephemeral artifacts. The coverage report in particular is
specific to one run on one machine and will go stale immediately.
- Recommendation: remove both markdown files from the PR. The analysis is valuable but belongs in
the PR description or a wiki, not committed to the repo. The coverage report is a point-in-time
artifact.
4. 3 branches still uncovered (B2a, B3a, `!equals`)
- The author identifies these gaps but doesn't close them. B2a is the most interesting — it's the
error path when delp has been cleared to NULL by a prior }} recovery and then no closing bracket is
found. This is exactly the kind of edge case that could hide a real bug.
- The suggested TC21/TC22 in the coverage report should be implemented before merge.
5. Test doesn't validate parsed values
- Every test case only checks "didn't crash." None verify that the parsed attribute/value pairs
are correct. For example, TC02 PWD={pass;word;123} should verify the password was parsed as
{pass;word;123} (with braces, since the function preserves them). A crash-only test is useful for
fuzzing but weak as a regression test.
6. TC07 behavior is suspicious
- The coverage report shows TC07 PWD={no_close_at_all hits "B2-recover × N" and returns with
authentication failure (not a parse error). This means the parser didn't detect the missing closing
bracket — it fell through to the connection attempt. That's arguably a bug in the parser itself,
and the test should flag it rather than marking it PASS.
7. Minor code issues in the test
- try_connect doesn't check return values of SQLAllocHandle/SQLSetEnvAttr — if these fail, the
subsequent SQLDriverConnect will crash on a null handle
- TC13 and TC17 are nearly identical (PWD={value}; vs PWD={val};) — they test the same branch
(B4-eof)
Verdict
The test cases themselves are solid work and the branch analysis is excellent. But the PR isn't
merge-ready:
- Add the test to test/tests and provide test/expected/bracket-parse.out
- Remove the Chinese markdown docs from the tree (put the analysis in the PR description)
- Add the 3 missing test cases for B2a/B3a/!equals
- Add at least basic value validation for the happy-path cases
- Investigate TC07's behavior — a missing closing bracket silently succeeding is worth a follow-up
issue
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-04-23 10:55 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-04-20 04:09 [postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing "jarvis24young (@jarvis24young)" <[email protected]>
2026-04-23 10:55 Re: [postgresql-interfaces/psqlodbc] PR #172: Add DT test cases and analysis for drvconn.c bracket parsing "davecramer (@davecramer)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox