[#222] test: 파서 property 기반 테스트 도입 (+ CR 공백 처리 1줄) - #242
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
Changes렉서 및 파서 테스트
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds CR whitespace support and expands parser/lexer regression coverage without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
리뷰 편의를 위해 확인한 내용 공유드립니다. 현재 열려 있는 제 PR 3건(#242, #243, #244)은 건드리는 파일이 서로 겹치지 않습니다.
실제로 셋을 모두 master 위에 합쳐서 확인했고, 충돌 없이 머지되며 테스트도 통과합니다: 어떤 순서로 병합하셔도 되고, 하나만 먼저 가져가셔도 나머지가 깨지지 않습니다. 부담되시면 제일 작은 #244(파일 1개)부터 보셔도 좋습니다. 각 PR이 고치는 내용은 이렇습니다:
세 건 모두 수정을 무력화하면 새 테스트가 실제로 실패하는 것을 확인했습니다. |
|
현재 열려 있는 제 PR 6건의 병합 가능성을 실제로 확인했습니다. master 위에 6건을 전부 순서대로 머지해본 결과, 충돌 없이 모두 적용되고 테스트도 통과합니다: 두 곳만 순서가 있습니다:
나머지는 순서와 무관하고, 하나만 먼저 가져가셔도 나머지가 깨지지 않습니다. 각 PR이 고치는 내용입니다:
전부 수정을 되돌리면 새 테스트가 실제로 실패하는 것을 확인했습니다. 부담되시면 파일 1개짜리(#244, #246, #247)부터 보셔도 됩니다. |
c8747f3 to
c5ae705
Compare
DPS0340
changed the title
c5ae705 to
42ee295
Compare
이전 실행이 코드와 무관한 러너 이슈로 실패했습니다: error: binary `cargo-tarpaulin` already exists in destination 동일 잡이 myyrakle#242/myyrakle#245/#246에서는 SUCCESS입니다.
42ee295 to
6c99986
Compare
이전 실행이 코드와 무관한 러너 이슈로 실패했습니다: error: binary `cargo-tarpaulin` already exists in destination 동일 잡이 myyrakle#242/myyrakle#245/#246에서는 SUCCESS입니다.
6c99986 to
f123302
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/parser/test/property.rs`:
- Around line 44-58: Update parser_never_panics_on_arbitrary_text and
parser_never_panics_on_sql_like_input to assert try_parse results instead of
discarding them. For valid SQL cases, assert successful parsing with is_ok();
for intentionally malformed cases, assert an error with the expected ErrorKind.
Preserve the panic-safety coverage while validating both successful parsing and
error-return contracts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10ee6158-72c6-4252-b1cd-dc6c6fd0ee0b
📒 Files selected for processing (6)
Cargo.tomlproptest-regressions/engine/parser/test/property.txtsrc/engine/lexer/test/eof_lookahead.rssrc/engine/lexer/tokenizer.rssrc/engine/parser/test/mod.rssrc/engine/parser/test/property.rs
| /// The parser must never panic, however malformed the input. Any input it | ||
| /// cannot handle has to come back as an `Err`, not an abort — a panic here | ||
| /// would take down the connection task that is parsing the statement. | ||
| #[test] | ||
| fn parser_never_panics_on_arbitrary_text(sql in arbitrary_sql_text()) { | ||
| let _ = try_parse(&sql); | ||
| } | ||
|
|
||
| /// Same invariant, but with inputs built from real SQL keywords so the | ||
| /// generator reaches the statement parsers rather than failing at the | ||
| /// tokenizer. | ||
| #[test] | ||
| fn parser_never_panics_on_sql_like_input(sql in sql_like_fragment()) { | ||
| let _ = try_parse(&sql); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
파싱 결과를 폐기하지 말고 성공·실패 계약을 단언하세요.
try_parse 결과를 버리므로 잘못된 SQL이 묵시적으로 성공해도, 유효 SQL이 실패해도 통과합니다. 유효 SQL 전략에는 is_ok()를, 의도적으로 잘못된 SQL 전략에는 기대한 ErrorKind의 오류를 단언하는 속성을 추가하세요. 이는 PR 목표의 “유효 SQL 파싱 성공” 및 “잘못된 SQL 오류 반환”을 현재 검증하지 못합니다.
Also applies to: 75-97
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/engine/parser/test/property.rs` around lines 44 - 58, Update
parser_never_panics_on_arbitrary_text and parser_never_panics_on_sql_like_input
to assert try_parse results instead of discarding them. For valid SQL cases,
assert successful parsing with is_ok(); for intentionally malformed cases,
assert an error with the expected ErrorKind. Preserve the panic-safety coverage
while validating both successful parsing and error-return contracts.
f090fb6 to
f799294
Compare
|
#250이 머지되면서 리베이스 후 로컬에서 전체 테스트를 다시 돌린 결과입니다: 참고로 #250이 고친 tarpaulin 캐시 경합이 바로 이 PR들의 coverage 잡을 간헐적으로 빨갛게 만들던 원인이었습니다. 이제 그 노이즈 없이 판단하실 수 있습니다. |
f799294 to
d82bb50
Compare
|
이 PR을 다시 보다가 제가 쓴 주석이 사실이 아니라는 것을 발견해서 고쳤습니다 ( 무엇이 잘못됐나
측정해보니 반대였습니다. 1024건을 생성해 실제로 문장이 파싱된 개수를 세면: 키워드를 균등하게 섞으면 생성된 입력을 찍어보면 바로 보입니다: 고친 것
1번만으로는 부족했습니다. 1.5%도 여전히 낮아서, 문장 본문을 제대로 훑으려면 유효한 문장에서 출발하는 쪽이 맞았습니다. 새 테스트가 실제로 무언가를 잡는지주장하지 않고 확인했습니다. 기존 5개는 전부 통과하고 신규 2개만 실패합니다. 이게 이 커밋의 요지입니다. 검증리베이스도 함께 했습니다. #250 머지 후 master 위에서 4개 체크 모두 초록불입니다. ( |
|
바로 앞 코멘트에서 한 말을 정정합니다. 제가 틀렸습니다. 이렇게 적었습니다:
master에서 재현되지 않습니다. 제가 만든 문제였습니다. 각 브랜치의
왜 놓쳤나master와 비교할 때 두 브랜치에서 같은 명령을 돌렸어야 했습니다. 실패하는 그 명령으로 대조했다면 즉시 드러났을 문제입니다. 앞 코멘트의 나머지 내용(생성기가 파서 본문에 도달하지 못하던 문제와 그 수정)은 그대로 유효합니다. |
|
반영했습니다 ( 확인한 것
전부 통과합니다. 말씀하신 대로 기존 property는 성공 계약을 전혀 검증하지 못하고 있었습니다. 반영패닉 방지 property 자체는 결과를 버리는 게 맞다고 봤습니다 — 임의 텍스트에 대해 성공/실패를 단정할 수 없고, 그 테스트의 계약은 "패닉하지 않는다" 하나입니다. 대신 빠져 있던 계약을 담당하는 property를 추가했습니다.
새 property가 실제로 무언가를 잡는지위의 "빈 결과만 돌려주는 파서"를 다시 넣었습니다: 신규 2건만 뒤집힙니다. 검증 |
584b2f0 to
218ab51
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/parser/test/property.rs`:
- Around line 146-148: Update parsing_a_mutated_statement_is_deterministic and
the related property tests in src/engine/parser/test/property.rs at lines
146-148, 154-162, and 167-175 to compare complete parse outcomes: preserve
successful AST equality and, for Err-Err results, explicitly verify matching
ErrorKind rather than only checking both are errors. Ensure the
whitespace-before/after failure property also validates identical ErrorKind; all
three sites require direct assertion updates.
- Around line 20-25: Expand the arbitrary input strategy in arbitrary_sql_text
so it generates genuinely arbitrary characters, including control characters,
quotes, and unbalanced delimiters, instead of the current restricted ASCII
regex. Use an appropriate proptest strategy such as proptest::char::any() or a
byte-based regex while preserving the String return type and existing
panic-invariant test usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: facd1785-4b17-45de-9257-8eee7d7ae849
📒 Files selected for processing (6)
Cargo.tomlproptest-regressions/engine/parser/test/property.txtsrc/engine/lexer/test/eof_lookahead.rssrc/engine/lexer/tokenizer.rssrc/engine/parser/test/mod.rssrc/engine/parser/test/property.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- proptest-regressions/engine/parser/test/property.txt
- src/engine/lexer/tokenizer.rs
- src/engine/parser/test/mod.rs
- src/engine/lexer/test/eof_lookahead.rs
218ab51 to
1aa4beb
Compare
|
CodeRabbit 리뷰 반영했습니다 (b003aa9). #3652563996 (Minor) — 임의 텍스트 생성 범위 #3652563997 (Minor) — Err/Err 결과 비교
#3651713224 (Major) 은 커밋 1aa4beb에서 이미 반영된 상태입니다( 검증: cargo test 745 passed / 0 failed, clippy 73건(master와 동일). |
proptest로 파서 불변식을 검증하는 테스트를 추가합니다. - 임의 입력에 대해 파서가 패닉하지 않음 - 공백만 있는 입력은 구문을 만들지 않음 - 왕복(파싱 → 재출력 → 재파싱) 일치 테스트를 돌리는 과정에서 `is_whitespace`가 캐리지 리턴(\r)을 공백으로 보지 않아, CRLF 줄바꿈 입력에서 파서가 멈추는 문제를 발견해 함께 고쳤습니다. 이 한 줄을 되돌리면 `whitespace_only_input_yields_no_statements`가 실제로 실패합니다. EOF 경계 처리는 #239와 중복이라 걷어내고, 이 브랜치를 myyrakle#239 위에 쌓았습니다. #239가 먼저 머지되어야 합니다.
이 PR의 주석에 '생성기가 파서 깊숙이 도달한다'고 적었는데, 측정해보니 사실이 아니었습니다. 1024건을 생성해 실제로 파싱되는 문장 수를 세면: sql_like_fragment nonempty_stmts = 0~1 / 1024 키워드를 균등하게 섞으면 문장을 여는 키워드로 시작하는 경우가 거의 없어서, 파서가 빈 문장 목록을 돌려주고 그 뒤 코드에는 진입조차 하지 못합니다. 즉 panic 불변식은 토크나이저 근처에서만 검증되고 있었습니다. 두 가지를 고쳤습니다. 1. sql_like_fragment의 첫 토큰을 문장을 여는 키워드로 고정 (0~1 -> 10~15) 2. mutated_statement 생성기 추가: 정상 문장에서 출발해 토큰 하나를 주입합니다. 파서가 문장 종류를 확정한 뒤에 예상치 못한 토큰을 만나므로 본문 코드에 도달합니다 (nonempty_stmts = ~480 / 1024) 새 테스트가 실제로 무엇을 잡는지 확인했습니다. parse_where에 panic을 심으면 - 유효한 SELECT 안에서만 도달하는 지점입니다: 기존 테스트 5개: 전부 통과 (놓침) 신규 테스트 2개: 전부 FAILED (잡음) cargo test 683 passed / 0 failed (기존 673 + 신규 2, proptest 케이스 증가분 포함). clippy 경고 70건으로 master와 동일.
f090fb6에서 Cargo.toml의 [[bench]] path를 ./src/benches/... 에서
./benches/... 로 바꿨는데, 의도한 변경이 아니었습니다. 이 PR은 proptest
dev-dependency만 추가하면 됩니다.
그 결과 cargo clippy --all-targets 가 이 브랜치에서만 실패했습니다:
error: can't find bench `index_benchmark` at path
/private/tmp/rrdb/benches/index_benchmark.rs
실제 파일은 src/benches/index_benchmark.rs 에 있고, master와 나머지 세
브랜치는 모두 원래 경로를 유지하고 있습니다. 원복 후 --all-targets 가
정상 완료되며 경고 수는 73건으로 master와 동일합니다.
CodeRabbit 리뷰 반영. 기존 property는 전부 '패닉하지 않는다'와 '결정적이다'만 확인하고 결과를 버렸습니다. 지적대로 유효 SQL이 실제로 파싱되는지는 아무도 검증하지 않고 있었습니다. 이게 왜 문제인지 실제로 확인했습니다. parse()가 무조건 빈 결과를 돌려주도록 바꿔봤더니: parser_never_panics_on_arbitrary_text ok parser_never_panics_on_sql_like_input ok parser_never_panics_on_mutated_statement ok parsing_is_deterministic ok '아무것도 파싱하지 않는 파서'가 기존 property를 전부 만족합니다. 두 가지를 추가했습니다. - valid_statement() 생성기 + valid_statements_parse_into_exactly_one_statement 구성상 유효한 SQL(SELECT/INSERT/UPDATE/DELETE/DROP 7형태, 세미콜론 유무 포함)이 파싱되고 정확히 한 문장을 만드는지 확인합니다. - a_bare_statement_keyword_is_an_error 실패 쪽 계약입니다. 키워드만 있고 뒤가 없는 입력은 조용히 빈 결과가 아니라 에러여야 합니다. bite-test: 위의 '빈 결과만 돌려주는 파서'를 넣으면 신규 2건만 FAILED로 뒤집히고 기존 패닉 property는 전부 통과합니다. cargo test 687 passed / 0 failed (직전 683 + 4). clippy --all-targets 73건으로 master와 동일.
CodeRabbit 리뷰(#3652563996, #3652563997) 반영. - arbitrary_sql_text를 proptest::char::any() 기반으로 교체. 기존 ASCII 정규식은 제어문자(NUL, 단독 \r)와 quote/unbalanced delimiter 경로를 만들지 못해 패닉 불변성 테스트 범위가 실제 임의 입력보다 좁았음. 1024 케이스 통과(파서 패닉 없음 유지). - 결정성 3건(parsing_a_mutated_statement_is_deterministic, parsing_is_deterministic, surrounding_whitespace_is_insignificant)이 Err/Err일 때 ErrorKind 동일성까지 비교. 기존에는 '둘 다 에러'면 종류가 달라도 통과했고, Ok/Err가 갈리면 조용히 통과했음. 이제 Ok/Err 엇갈림은 실패로 처리. cargo test 745 passed / 0 failed, clippy 73건(master와 동일)
b003aa9 to
c76c0b5
Compare
2 participants
resolves: #222 (Phase 1-2)
요약
이슈의 Phase 1(인프라)과 Phase 2(파서) 를 구현했습니다. Phase 3-5(인덱스/옵티마이저/스토리지)는 범위가 크고 진행 중인 인덱스 작업과 파일이 겹쳐서 후속으로 분리했습니다.
#239를 제외하면 이 PR의 실제 변경은 소스 한 줄과 테스트입니다.
유일한 소스 변경: CR(
\r)이 공백으로 인식되지 않음is_whitespace가' ',\n,\t만 처리해서, CRLF 줄바꿈이 든 SQL이unexpected character: '\r'로 실패했습니다. Windows에서 저장한 스크립트나 CRLF를 보내는 클라이언트가 영향을 받습니다.속성 테스트를 돌리다 발견한 버그입니다. 이 한 줄을 되돌리면
whitespace_only_input_yields_no_statements가 실제로 실패하는 것을 확인했습니다:추가한 테스트
engine/parser/test/property.rs— proptest 기반 속성 5종engine/lexer/test/eof_lookahead.rs— #239가 고친 EOF 경계를 입력 끝 연산자(-,/,<,>,!)별로 고정하는 회귀 테스트입니다. 수정 자체는 #239 소유이고, 여기서는 그 동작이 유지되는지만 검증합니다.확인
Summary by CodeRabbit
테스트
스타일