Skip to content
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

In CI print logs only when any test in a class fails #10620

Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -44,16 +44,24 @@
import io.trino.util.AutoCloseableCloser;
import org.assertj.core.api.AssertProvider;
import org.intellij.lang.annotations.Language;
import org.testng.ITestContext;
import org.testng.SkipException;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.function.Consumer;
import java.util.function.Function;
import java.util.logging.ConsoleHandler;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.logging.MemoryHandler;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
Expand All @@ -75,29 +83,84 @@ public abstract class AbstractTestQueryFramework
private SqlParser sqlParser;
private final AutoCloseableCloser afterClassCloser = AutoCloseableCloser.create();
private io.trino.sql.query.QueryAssertions queryAssertions;
private Map<Handler, Level> oldLevels;
private MemoryHandler memoryHandler;

@BeforeClass
public void init()
throws Exception
{
enableLogBuffering();
queryRunner = afterClassCloser.register(createQueryRunner());
h2QueryRunner = afterClassCloser.register(new H2QueryRunner());
sqlParser = new SqlParser();
queryAssertions = new io.trino.sql.query.QueryAssertions(queryRunner);
}

private void enableLogBuffering()
{
if (System.getenv("CONTINUOUS_INTEGRATION") == null) {
return;
}
// change the log level of the console handler
Logger rootLogger = Logger.getLogger("");
Handler[] handlers = rootLogger.getHandlers();
oldLevels = new HashMap<>();
Handler consoleHandler = null;
for (Handler handler : handlers) {
if (!(handler instanceof ConsoleHandler)) {
continue;
}
oldLevels.put(handler, handler.getLevel());
handler.setLevel(Level.OFF);
Comment on lines +114 to +115
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging is global, and tests execute in parallel.

  • Concurrently to a test extending from ATQF, there might be some other test that doesn't, and we switch its logs off
  • when two instances of ATQF initialize concurrently, the second one may record oldLevels as OFFs, and dutifully restore them. This would disable logging for ever, for all tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What controls this? I naively thought parallelism is limited to classes: https://github.com/trinodb/trino/blob/master/pom.xml#L79

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

methods probably means that any two methods can run in parallel, so including two methods for same class, for same test instance, and for different classes and instances
classes probably means that methods from a single class are not supposed to run in parallel (not sure how this plays with class hierarchies)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found this article that has some examples with multiple classes and looks like you're right. I'll close this PR then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if i weren't right at least to some extent, i wouldn't spend time on apache/maven-surefire#407 :)

consoleHandler = handler;
}

// add a capturing handler
if (consoleHandler != null) {
memoryHandler = new MemoryHandler(consoleHandler, 1_000_000, Level.OFF);
rootLogger.addHandler(memoryHandler);
}
}

protected abstract QueryRunner createQueryRunner()
throws Exception;

@AfterClass(alwaysRun = true)
public final void close()
public final void close(ITestContext context)
throws Exception
{
afterClassCloser.close();
queryRunner = null;
h2QueryRunner = null;
sqlParser = null;
queryAssertions = null;
flushBufferedLogs(context);
}

private void flushBufferedLogs(ITestContext context)
{
if (System.getenv("CONTINUOUS_INTEGRATION") == null) {
return;
}
if (oldLevels.size() == 0) {
return;
}
// restore the log level of the console handler
for (Handler handler : Logger.getLogger("").getHandlers()) {
if (!(handler instanceof ConsoleHandler) || !oldLevels.containsKey(handler)) {
continue;
}
handler.setLevel(oldLevels.get(handler));
}
if (memoryHandler == null) {
return;
}
if (context.getFailedTests().size() != 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an equivalent to this work for JUnit 5? Otherwise, we’ll lose this functionality when we migrate these tests to it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check. I was not aware if we prefer JUnit 5 or TestNG.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a TestWatcher. Is there any existing JUnit 5 test that has a Query Runner or logs something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’re trying to move to JUnit 5: #9378

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AbstractTestQueryFramework fundamentally requires TestNG due to the annotations in the base class, so this would have to be updated when we migrate it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but I wouldn’t want to add dependencies that will make it harder or introduce a roadblock to migrating.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, maybe I can start the migration now? Would this be a good enough reason?

memoryHandler.push();
}
// remove it but don't close it, since it would also close the target (console) handler
Logger.getLogger("").removeHandler(memoryHandler);
}

@Test
Expand Down