test: expand stdlib coverage suites #1365
Conversation
Summary of ChangesHello @cpunion, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for the llgo compiler's compatibility with the Go standard library. It introduces a new chore utility to ensure comprehensive test coverage of exported symbols, establishes clear guidelines for writing and organizing standard library tests, and provides initial, extensive test suites for the math and strings packages. These changes are crucial for validating llgo's adherence to Go's standard library specifications and facilitating future contributions to test coverage. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new tool for checking standard library symbol coverage, adds comprehensive tests for the math and strings packages, and fixes an issue with math.Gamma. The new tooling is a valuable addition for maintaining test coverage. The approach of parsing go doc output is reasonable for an internal tool, though it can be fragile. I've identified a bug in the identifier parsing logic and an area where error handling could be improved. The new tests and the math.Gamma fix are well-implemented.
| func parseIdentifier(input string) string { | ||
| for i, r := range input { | ||
| if !(unicode.IsLetter(r) || unicode.IsDigit(r)) { | ||
| if i == 0 { | ||
| return "" | ||
| } | ||
| return input[:i] | ||
| } | ||
| } | ||
| return input | ||
| } |
There was a problem hiding this comment.
The current implementation of parseIdentifier does not correctly handle identifiers containing underscores (_). It will truncate the identifier at the first underscore. For example, an identifier like My_Func would be incorrectly parsed as My. This could lead to inaccurate coverage reports for symbols with underscores.
To fix this, the loop condition should also allow underscores.
| func parseIdentifier(input string) string { | |
| for i, r := range input { | |
| if !(unicode.IsLetter(r) || unicode.IsDigit(r)) { | |
| if i == 0 { | |
| return "" | |
| } | |
| return input[:i] | |
| } | |
| } | |
| return input | |
| } | |
| func parseIdentifier(input string) string { | |
| for i, r := range input { | |
| if !(unicode.IsLetter(r) || unicode.IsDigit(r) || r == '_') { | |
| if i == 0 { | |
| return "" | |
| } | |
| return input[:i] | |
| } | |
| } | |
| return input | |
| } |
Code Review SummaryExcellent work on the stdlib testing infrastructure! The test organization is well-structured and the coverage checker is a valuable addition. Key Strengths:
Priority Issues to Address🔴 Security Issues1. Command Injection Risk (
2. Path Traversal Risk (
⚡ Performance Issues3. Redundant File Parsing (
4. Repeated
5. Redundant CI Runs (
📝 Code Quality Issues6. Complex Monolithic Function (
7. Incorrect Identifier Parsing (
8. Global Mutable State (
📚 Documentation Issues9. Non-existent Directory Reference (
10. Benchmark Quality (
Overall Assessment: This PR adds valuable infrastructure with good test organization. The main concerns are security validations in the checker tool and some performance optimizations. The testing approach is solid and well-documented. |
cpunion
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1365 +/- ##
=======================================
Coverage 90.58% 90.58%
=======================================
Files 43 43
Lines 11429 11429
=======================================
Hits 10353 10353
Misses 914 914
Partials 162 162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cpunion
changed the title
cpunion
changed the title
cpunion
force-pushed
the
std-test
branch
4 times, most recently
from
3430f66 to
570888c
Compare
cpunion
force-pushed
the
std-test
branch
3 times, most recently
from
d4ba40a to
aafafe2
Compare
xgopilot
bot
force-pushed
the
std-test
branch
2 times, most recently
from
c7ac17a to
14d2dde
Compare
cpunion
force-pushed
the
std-test
branch
8 times, most recently
from
bff585e to
ec87b98
Compare
- Add darwin and generic unix signal backends for os/signal\n- Route clite/signal through a C sigaction helper for ABI safety\n- Fix syscall.Kill to use libc kill(2)\n- Mark os/signal std suite green
1 participant
Summary
doc/_readme/scripts/check_std_cover.shhelper that auto-discoverstest/stdsuitesValidation
go test ./test/std/...doc/_readme/scripts/check_std_cover.sh./llgo.sh test ./test/std/...Coverage Status
Core Collections & Strings
bytes— 95/95cmp— 4/4iter— 4/4 (fails under llgo runtime - depens coro)maps— 10/10slices— 40/40sort— 36/36strings— 80/80strconv— 40/40Math & Numerics
math— 97/97math/big— 154/154math/bits— 50/50 (llgo test fails in 15s sweep: expected panic on division by zero)math/rand— 37/37math/rand/v2— 54/54Containers & Utilities
container/heap— 6/6container/list— 19/19Archives & Compression
archive/tar— 33/33 (llgo test fails: ErrInsecurePath )archive/zip— 36/36 (llgo test fails: exit code -1)compress/flate— 22/22 (llgo test fails in 15s sweep)compress/gzip— 18/18 (llgo test fails in 15s sweep)compress/zlib— 16/16 (llgo test fails in 15s sweep)I/O & Filesystems
bufio— 51/51embed— 4/4 (llgo test fails in 15s sweep)io— 59/59 (llgo test timed out in 15s sweep)io/fs— 22/22 (llgo test fails in 15s sweep)io/ioutil— 8/8 (llgo compile fails in 15s sweep)os— 157/157 (llgo compile fails in 15s sweep)os/exec— 7/7 (llgo test fails: todo File.WriteString)os/signal— 6/6 (llgo test fails: signal_enable not implemented)os/user— 11/11 (llgo test fails: exit code -1)path— 9/9path/filepath— 27/27 (llgo test timed out in 15s sweep)Encoding & Serialization
encoding— 6/6encoding/asn1— 42/42 (llgo compile fails: undefined symbol reflect.Copy)encoding/base64— 21/21encoding/binary— 22/22encoding/csv— 17/17encoding/hex— 15/15 (llgo test fails in 15s sweep)encoding/json— 48/48 (llgo test fails in 15s sweep)encoding/pem— 3/3Text & Unicode
text/scanner— 29/29 (llgo test fails — see text/scanner sentinel is treated as NUL under llgo #1366)text/tabwriter— 11/11text/template— 29/29 (llgo test timed out; runtime gap — see llgo cannot compile unique.Make demo #1358)text/template/parse— 82/82unicode— 292/292unicode/utf16— 7/7 (llgo test fails: Decode mismatch)unicode/utf8— 19/19 (llgo test fails: DecodeLastRune*)unique— 2/2Time & Scheduling
time— 101/101Crypto & Security
crypto— 13/13crypto/aes— 4/4 (llgo compile fails)crypto/cipher— 9/9 (llgo compile fails)crypto/des— 5/5 (llgo compile fails)crypto/dsa— 9/9 (llgo compile fails)crypto/ecdh— 12/12 (llgo compile fails)crypto/ecdsa— 12/12 (llgo compile fails)crypto/ed25519— 17/17 (llgo compile fails)crypto/elliptic— 13/13 (llgo compile fails)crypto/hmac— 2/2 (llgo test fails: hmac.New unsupported)crypto/md5— 4/4crypto/rand— 5/5 (llgo compile fails)crypto/rc4— 5/5 (llgo compile fails)crypto/rsa— 30/30 (llgo compile fails)crypto/sha1— 4/4crypto/sha256— 7/7 (llgo test fails: hash length = 32, want 28)crypto/sha3— 23/23 (llgo compile fails: missing fips140 symbols)crypto/sha512— 13/13 (llgo test fails: hash length = 64, want 48)crypto/subtle— 8/8 (llgo compile fails: missing runtime.* and XORBytes)crypto/tls— 113/113 (llgo compile fails; runtime gap — see llgo cannot compile unique.Make demo #1358)crypto/x509— 76/76 (llgo compile fails)crypto/x509/pkix— 15/15Hashing
hash— 11/11hash/adler32— 10/10hash/crc32— 11/11hash/maphash— 16/16Networking & HTTP
net— 167/266 (llgo compile fails; runtime gap — see llgo cannot compile unique.Make demo #1358)net/http— 238/238 (llgo compile fails; runtime gap — see llgo cannot compile unique.Make demo #1358)net/http/httptest— 17/17 (llgo compile fails; runtime gap — see llgo cannot compile unique.Make demo #1358)net/http/httptrace— 6/6 (llgo compile fails; runtime gap — see llgo cannot compile unique.Make demo #1358)net/netip— 63/63net/textproto— 36/36net/url— 41/41Concurrency & Context
context— 13/13 (llgo test fails in 15s sweep)sync— 40/40 (llgo compile fails in 15s sweep)sync/atomic— 94/94Language Tooling
go/ast— 0/?go/build— 16/16go/build/constraint— 19/19go/constant— 14/14go/doc— 0/?go/doc/comment— 0/?go/importer— 0/?go/parser— 5/5 (llgo test timed out in 15s sweep)go/scanner— 18/18go/token— 45/45go/types— 0/?go/version— 3/3Runtime & System
runtime— 62/62 (llgo test fails: TestTypeAssertionError/TestCleanup)runtime/debug— 18/18 (llgo compile fails in 15s sweep)runtime/pprof— 15/15 (llgo compile fails in 15s sweep)runtime/trace— 10/10 (llgo compile fails in 15s sweep)syscall— 0/? SKIPunsafe— 8/8weak— 2/2Testing & Benchmarks
testing— 108/108 (llgo test timed out in 15s sweep)testing/fstest— 9/9 (llgo test fails in 15s sweep)testing/iotest— 10/10Application & Formatting
errors— 6/6flag— 74/74fmt— 29/29log— 41/41mime— 13/13mime/multipart— 7/7 (llgo compile fails in 15s sweep)mime/quotedprintable— 5/5reflect— 106/106 (llgo compile fails in 15s sweep; missing ChanDir.String, Copy, Swapper)regexp— 45/45regexp/syntax— 10/10