Skip to content

Reject @BeforeMethod without singleThreaded=true #15756

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.

Sign up for GitHub

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

Closed
v-jizhang wants to merge 1 commit into from
Closed

Reject @BeforeMethod without singleThreaded=true #15756

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 16 additions & 15 deletions ...o-benchmark-runner/src/test/java/com/facebook/presto/benchmark/retry/TestRetryDriver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import com.facebook.airlift.log.Logging;
import com.facebook.presto.benchmark.framework.QueryException;
import io.airlift.units.Duration;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import java.net.SocketTimeoutException;
Expand Down Expand Up @@ -64,23 +63,10 @@ public Integer run()
private static final QueryException RETRYABLE_EXCEPTION = QueryException.forClusterConnection(new SocketTimeoutException());
private static final QueryException NON_RETRYABLE_EXCEPTION = QueryException.forPresto(new RuntimeException(), Optional.of(REMOTE_HOST_GONE), Optional.empty());

private RetryDriver retryDriver;

@BeforeMethod
public void setup()
Comment thread
aweisberg marked this conversation as resolved.
Outdated
{
retryDriver = new RetryDriver(
new RetryConfig()
.setMaxAttempts(5)
.setMinBackoffDelay(new Duration(10, MILLISECONDS))
.setMaxBackoffDelay(new Duration(100, MILLISECONDS))
.setScaleFactor(2),
exception -> (exception instanceof QueryException) ? (((QueryException) exception).getType() == CLUSTER_CONNECTION) : FALSE);
}

@Test
public void testSuccess()
{
RetryDriver retryDriver = getRetryDriverForMethods();
assertEquals(
retryDriver.run("test", new MockOperation(5, RETRYABLE_EXCEPTION)),
Integer.valueOf(5));
Expand All @@ -89,12 +75,14 @@ public void testSuccess()
@Test(expectedExceptions = QueryException.class)
public void testMaxAttemptsExceeded()
{
RetryDriver retryDriver = getRetryDriverForMethods();
retryDriver.run("test", new MockOperation(6, RETRYABLE_EXCEPTION));
}

@Test(expectedExceptions = QueryException.class)
public void testNonRetryableFailure()
{
RetryDriver retryDriver = getRetryDriverForMethods();
retryDriver.run("test", new MockOperation(3, NON_RETRYABLE_EXCEPTION));
}

Expand All @@ -110,4 +98,17 @@ public void testBackoffTimeCapped()
exception -> (exception instanceof QueryException) ? (((QueryException) exception).getType() == CLUSTER_CONNECTION) : FALSE);
retryDriver.run("test", new MockOperation(5, RETRYABLE_EXCEPTION));
}

private RetryDriver getRetryDriverForMethods()
{
RetryDriver retryDriver = new RetryDriver(
new RetryConfig()
.setMaxAttempts(5)
.setMinBackoffDelay(new Duration(10, MILLISECONDS))
.setMaxBackoffDelay(new Duration(100, MILLISECONDS))
.setScaleFactor(2),
exception -> (exception instanceof QueryException) ? (((QueryException) exception).getType() == CLUSTER_CONNECTION) : FALSE);

return retryDriver;
}
}
16 changes: 8 additions & 8 deletions presto-main/src/test/java/com/facebook/presto/execution/TestThriftTaskStatus.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import com.facebook.presto.util.Failures;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.BeforeTest;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

Expand Down Expand Up @@ -77,13 +77,7 @@ public class TestThriftTaskStatus
public static final int TOTAL_CPU_TIME_IN_NANOS = 1002;
public static final int TASK_AGE = 1003;
public static final HostAddress REMOTE_HOST = HostAddress.fromParts("www.fake.invalid", 8080);
private TaskStatus taskStatus;

@BeforeMethod
public void setUp()
{
taskStatus = getTaskStatus();
}
private static TaskStatus taskStatus;

@DataProvider
public Object[][] codecCombinations()
Expand All @@ -96,6 +90,12 @@ public Object[][] codecCombinations()
};
}

@BeforeTest
public void setup()
{
taskStatus = getTaskStatus();
}

@Test(dataProvider = "codecCombinations")
public void testRoundTripSerializeBinaryProtocol(ThriftCodec<TaskStatus> readCodec, ThriftCodec<TaskStatus> writeCodec)
throws Exception
Expand Down
8 changes: 4 additions & 4 deletions presto-parser/src/test/java/com/facebook/presto/sql/parser/ParsingExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,17 @@
package com.facebook.presto.sql.parser;

import com.facebook.presto.sql.tree.NodeLocation;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.BeforeTest;
import org.testng.annotations.Test;

import static org.testng.Assert.assertEquals;

public class ParsingExceptionTest
{
private ParsingException parsingException;
private static ParsingException parsingException;

@BeforeMethod
public void setUp()
@BeforeTest
public void setup()
{
parsingException = new ParsingException("ParsingException", new NodeLocation(100, 1));
}
Expand Down
6 changes: 3 additions & 3 deletions ...verifier/src/test/java/com/facebook/presto/verifier/framework/TestQueryConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
package com.facebook.presto.verifier.framework;

import com.google.common.collect.ImmutableMap;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.BeforeTest;
import org.testng.annotations.Test;

import java.util.Map;
Expand Down Expand Up @@ -50,9 +50,9 @@ public class TestQueryConfiguration
Optional.of(PASSWORD_OVERRIDE),
Optional.of(SESSION_PROPERTIES_OVERRIDE));

private QueryConfigurationOverridesConfig overrides;
private static QueryConfigurationOverridesConfig overrides;

@BeforeMethod
@BeforeTest
public void setup()
{
overrides = new QueryConfigurationOverridesConfig()
Expand Down
28 changes: 14 additions & 14 deletions ...erifier/src/test/java/com/facebook/presto/verifier/framework/TestVerificationManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;

import java.util.ArrayList;
Expand Down Expand Up @@ -120,19 +119,13 @@ public List<VerifierQueryEvent> getEvents()
QUERY_CONFIGURATION);
private static final VerifierConfig VERIFIER_CONFIG = new VerifierConfig().setTestId("test");

private MockEventClient eventClient;

@BeforeMethod
public void setup()
{
this.eventClient = new MockEventClient();
}

// TODO(leiqing): Add a test where the first submission fails but resubmission succeeds.
@Test
public void testFailureResubmitted()
{
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(HIVE_PARTITION_DROPPED_DURING_QUERY), VERIFIER_CONFIG);
MockEventClient eventClient = new MockEventClient();
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(HIVE_PARTITION_DROPPED_DURING_QUERY),
VERIFIER_CONFIG, eventClient);
manager.start();
assertEquals(manager.getQueriesSubmitted().get(), 3);
assertEquals(eventClient.getEvents().size(), 1);
Expand All @@ -142,7 +135,9 @@ public void testFailureResubmitted()
@Test
public void testFailureNotSubmitted()
{
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(GENERIC_INTERNAL_ERROR), VERIFIER_CONFIG);
MockEventClient eventClient = new MockEventClient();
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(GENERIC_INTERNAL_ERROR),
VERIFIER_CONFIG, eventClient);
manager.start();
assertEquals(manager.getQueriesSubmitted().get(), 1);
assertEquals(eventClient.getEvents().size(), 1);
Expand All @@ -152,6 +147,7 @@ public void testFailureNotSubmitted()
@Test
public void testFilters()
{
MockEventClient eventClient = new MockEventClient();
List<SourceQuery> queries = ImmutableList.of(
createSourceQuery("q1", "CREATE TABLE t1 (x int)", "CREATE TABLE t1 (x int)"),
createSourceQuery("q2", "CREATE TABLE t1 (x int)", "CREATE TABLE t1 (x int)"),
Expand All @@ -165,7 +161,8 @@ public void testFilters()
new VerifierConfig()
.setTestId("test")
.setWhitelist("q2,q3,q4,q5,q6")
.setBlacklist("q2"));
.setBlacklist("q2"),
eventClient);
manager.start();
assertEquals(manager.getQueriesSubmitted().get(), 0);

Expand All @@ -180,7 +177,9 @@ public void testFilters()
@Test
public void testVerifierError()
{
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(new RuntimeException()), VERIFIER_CONFIG);
MockEventClient eventClient = new MockEventClient();
VerificationManager manager = getVerificationManager(ImmutableList.of(SOURCE_QUERY), new MockPrestoAction(new RuntimeException()),
VERIFIER_CONFIG, eventClient);
manager.start();

List<VerifierQueryEvent> events = eventClient.getEvents();
Expand All @@ -202,7 +201,8 @@ private static void assertSkippedEvent(VerifierQueryEvent event, String name, Sk
assertEquals(event.getSkippedReason(), skippedReason.name());
}

private VerificationManager getVerificationManager(List<SourceQuery> sourceQueries, PrestoAction prestoAction, VerifierConfig verifierConfig)
private VerificationManager getVerificationManager(List<SourceQuery> sourceQueries, PrestoAction prestoAction,
VerifierConfig verifierConfig, MockEventClient eventClient)
{
return new VerificationManager(
() -> sourceQueries,
Expand Down
4 changes: 4 additions & 0 deletions src/checkstyle/presto-checks.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@
<property name="format" value="^[ \t]*import static org\.testng\.AssertJUnit\." />
<property name="message" value="Use org.testng.Assert instead of org.testng.AssertJUnit" />
</module>
<module name="RegexpMultiline">
<property name="format" value="(?&lt;!@Test\(singleThreaded[ ]?=[ ]?true\))\npublic class \w+\n\{\n(.*\n){0,200}\s+@BeforeMethod\n" />
Copy link
Copy Markdown
Contributor

@aweisberg aweisberg Mar 16, 2021

Choose a reason for hiding this comment

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

Could you use a simpler regex that is less likely to fail if the @BeforeMethod is later in the class.
Such as check for the BeforeMethod import (guaranteed to come first) and then singleThreaded=True?

How reliable is this regex going to be? Seems like several places could have whitespace that aren't accounted for in the regex. I know our style checks forbid a lot of them, but I just want to check in and find out what you tested?

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Mar 16, 2021

Choose a reason for hiding this comment

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

Could you use a simpler regex that is less likely to fail if the @BeforeMethod is later in the class.
Such as check for the BeforeMethod import (guaranteed to come first) and then singleThreaded=True?

Good idea, I'll try that.

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Mar 16, 2021

Choose a reason for hiding this comment

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

How reliable is this regex going to be? Seems like several places could have whitespace that aren't accounted for in the regex. I know our style checks forbid a lot of them, but I just want to check in and find out what you tested?

Yes, white spaces are checked by other style checks.

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Mar 16, 2021

Choose a reason for hiding this comment

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

Could you use a simpler regex that is less likely to fail if the @BeforeMethod is later in the class.
Such as check for the BeforeMethod import (guaranteed to come first) and then singleThreaded=True?

This can't be a simple regex. if BeforeMethod import exists, the test class must be annotated by singleThreaded=True. But we check for violations, that is, BeforeMethod import exists, but singleThreaded=True does not. That is negative LookBefore.

Copy link
Copy Markdown
Contributor

@aweisberg aweisberg Mar 17, 2021

Choose a reason for hiding this comment

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

If we can match on the acceptable method can we negate it with something like

    static String negateRegex(String regex) {
        return "(?!(?:" + regex + ")$).*";
    }

My regex skills are not that strong. Seems like negating a regex is generally possible.

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Mar 18, 2021

Choose a reason for hiding this comment

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

Looks like it is as complicated as negative lookbehind. And ".*" at the end consumes everything of a file, for a big file, the regex engine could raise a stack overflow exception.

<property name="message" value="@BeforeMethod cannot be used in a class that is not @Test(singleThreaded=true)" />
</module>

<module name="SuppressWarningsFilter" />

Expand Down