Deliver the interpreter output of tests to RunTests - #1205
Conversation
RunTests.redirectInterpreterOutput never received anything, so whatever a test printed went to System.err instead of the stream RunTests writes to. As a consequence -compactOutput could not suppress that output either. Three independent reasons, all fixed here: - ProgramStateIO shadowed ProgramState.outStream with its own field and overrode getOutStream/setOutStream without forwarding to the native providers. Removed, the base class already implements both correctly. - ReflectionNativeProvider.setOutStream was an empty method, so OutputProvider, which implements println/BJDebugMsg/DisplayTextToPlayer and friends, kept writing to its default System.err. - The PrintStream wrapping the redirect had no autoflush, so its buffer was never emptied. It now autoflushes and uses an explicit UTF-8 charset on both the encoding and the decoding side.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4427ebbdd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| public void write(byte[] b, int off, int len) throws IOException { | ||
| if (!compactOutput) { | ||
| println(new String(b, off, len)); | ||
| println(new String(b, off, len, StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
In non-compact runs, every output native calls PrintStream.println, and PrintStream passes bytes that already contain the line separator to this bulk-write method. Calling RunTests.println here appends another separator, so each interpreter output line is delivered with an unintended blank line; forward the decoded chunk with print instead.
Useful? React with 👍 / 👎.
2 participants
RunTests.redirectInterpreterOutputinstalls a stream so that whatever the tests printgoes to
RunTests' own output. That stream never received anything: the output wentstraight to
System.err, bypassingRunTestsentirely. A side effect is that-compactOutputcannot suppress it either, so it still shows up in CI logs.Three independent reasons, each fixed here:
ProgramStateIOshadows the stream. It declares its ownprivate PrintStream outStreamand overridesgetOutStream/setOutStreamon top ofProgramState, whichalready has both. The base implementation forwards the new stream to every registered
NativesProvider; the override did not, so the providers kept their old stream. Thefield and the two overrides are removed and the base class does the work.
ReflectionNativeProvider.setOutStreamis empty. It is the provider that ownsOutputProvider, which implementsprintln,BJDebugMsg,DisplayTextToPlayerand theother printing natives. Because the setter did nothing,
OutputProviderkept itsdefault
System.err. It now forwards to theOutputProviderinstances it holds, andOutputProvidergets the corresponding setter.The
PrintStreamhas no autoflush. Even with the two fixes above, output sat in the8 KB buffer of
new PrintStream(os)and was never flushed, so a normal test rundelivered nothing. It now autoflushes, with an explicit UTF-8 charset on the encoding
side and the matching
new String(bytes, UTF_8)on the decoding side, which also fixesnon-ASCII output being decoded with the platform default charset.
Effect
Output printed by tests during
-runtestsnow reaches the streamRunTestsprints to(stdout on the CLI) instead of
System.err, which is what the redirection was written todo, and
-compactOutputsuppresses it as intended.Tests
RunTestsOutputRedirectTestscompiles a package with one passing and one failing test thatboth print, and asserts the printed output arrives in
RunTests' sink, that-compactOutputsuppresses it, and that the summary and assertion message are unaffected.interpreterOutputIsRedirectedfails on unmodified master and passes with this change;the full
./gradlew testsuite passes.