-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test product tests launcher #17875
Test product tests launcher #17875
Conversation
This one is for you @kokosing 😇 |
c9cf747
to
ccbfad7
Compare
...ct-tests-launcher/src/main/java/io/trino/tests/product/launcher/cli/EnvironmentDescribe.java
Show resolved
Hide resolved
...uct-tests-launcher/src/main/java/io/trino/tests/product/launcher/env/EnvironmentOptions.java
Outdated
Show resolved
Hide resolved
Nice! |
This parameter is null by default and is only initialized by picocli when the command is invoked.
This prevents regressions where some of the invocations are broken because of the lack of the dependencies or null derefences.
ccbfad7
to
d01c513
Compare
@@ -232,7 +230,7 @@ public Execution( | |||
this.outputBuilder = describeOptions.format.outputBuilderFactory.get(); | |||
|
|||
try { | |||
this.out = new PrintStream(new FileOutputStream(FileDescriptor.out), true, Charset.defaultCharset().name()); | |||
this.out = new PrintStream(System.out, true, Charset.defaultCharset().name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is causing to actually write to stderr because of https://github.com/airlift/airlift/blob/master/log-manager/src/main/java/io/airlift/log/Logging.java#L139
This commit should be reverted. Logging to stderr here makes this fail to read any output: https://github.com/trinodb/trino/blob/master/.github/bin/build-pt-matrix-from-impacted-connectors.py#L108
and in result we're always running all PTs in PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaces #17873