Jens-G opened a new pull request, #3913: URL: https://github.com/apache/thrift/pull/3913
Open since May. `lib/st` has no CI, so nothing runs its tests — and it has two SUnit suites that, until now, had never been run by anything but the person who wrote them: - `TProtocolStringSizeLimitTest` (11 tests), added with the string size limit - `TProtocolRecursionDepthTest` (6 tests), added with the recursion depth limit Both pass on an unmodified tree. That is worth knowing on its own. ## Why this job doesn't look like the others `lib/st` can't be wired up the way `lib-d` or `lib-lua` are. It isn't in `configure.ac` and isn't in a `SUBDIRS`, so **there is no `make check` to hang a job on** — the binding is a Squeak/Pharo file-in and nothing else. `lib-javame` is the closest existing precedent. GNU Smalltalk is no help: it cannot parse the chunk format `thrift.st` is written in, which is what the old "not loadable" impression came from. That's a fact about *gst*, not about the binding — **Pharo files it in cleanly**, so that is what the job uses. ## The script does the asserting, because Pharo won't `lib/st/test/run-tests.sh` exists so a developer runs exactly what CI runs. It files in `thrift.st` and the suites, generates the code `TProtocolRecursionDepthTest` needs, runs both, and reports. It parses the counts rather than trusting the exit status, and that isn't defensive programming — I checked: ``` $ pharo Pharo.image eval "<a suite with one deliberately failing test>" 'RUN=1 PASS=0 FAIL=1' $ echo $? 0 ``` **Pharo exits 0 on a failed test.** So the counts are the only signal there is. It checks the number of tests that **ran**, not just that none failed. A suite that fails to file in registers no tests and would otherwise report `RUN=0 PASS=0 FAIL=0` — zero failures, reads as success. ## Both assertions verified by mutation | mutation | result | |---|---| | remove the `checkStringSize:` call from `readString` | `RUN=11 PASS=6 FAIL=4 ERROR=1` → job fails | | expected count made wrong (stands in for a partial file-in) | `ran 11 tests, expected 99` → job fails | ## Notes for review - **Pharo version is pinned to the `130` line**, not `stable`. `stable` will move to Pharo 14 one day and turn this job red for reasons unrelated to Thrift. The job prints `Smalltalk version` so drift is visible in the log. - The `EXPECTED` table at the top of the script needs updating when a test is added; the failure message says so explicitly. - `lib/Makefile.am` already lists `st` in `EXTRA_DIST` as a whole directory, so the new script ships with the tarball without further changes. - No new third-party Actions — `actions/checkout` and `actions/download-artifact`, both already used here. `zizmor` clean. With this, `lib/st` is no longer a binding whose tests nobody runs — which also gives [THRIFT-6258](https://issues.apache.org/jira/browse/THRIFT-6258) and [THRIFT-6300](https://issues.apache.org/jira/browse/THRIFT-6300) somewhere to prove a fix. Those follow separately. JIRA: [THRIFT-6063](https://issues.apache.org/jira/browse/THRIFT-6063) 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
