Reject @BeforeMethod without singleThreaded=true

[v-jizhang]

@BeforeMethod in a class that is not @test(singleThreaded=true) is
usually (always?) a bug. We should automatically find and reject them.

This change uses Regex negative LookBehind match to find @BeforeMethod
in a class that is not @test(singleThreaded=true). Only top 200 lines
of a class is scanned to save time and avoid potential stack overflow
if a file is too big. This works because @BeforeMethod should appear at
top a class definition.

Some violations have been caught and are fixed in this change too.

Resolves: #12044

== NO RELEASE NOTE ==

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Jinlin Zhang (14197cd4222f1ac115d4380c00cc9a185b73941b)

[aweisberg]

Some feedback on thread safety and questions about the regex

[aweisberg]

This is immutable for the duration of the test so it can be static (and final?) and constructed in a @BeforeTest method.

[v-jizhang]

It can be static. To construct it @BeforeTest, it can't be final. I'll make the change. Thanks

[aweisberg]

Another immutable thing. You can switch to @BeforeTest and make it static.

[aweisberg]

Also immutable, static and @BeforeTest

[aweisberg]

Remove this field. MockEventClient is not thread safe. Each test will need to pass this around on the stack or as a thread local (don't recommend that here).

[aweisberg]

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?

[v-jizhang]

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.

[v-jizhang]

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.

[v-jizhang]

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.

[aweisberg]

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.

[v-jizhang]

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.

[aweisberg]

Please squash the commits. I do appreciate changes during review being represented as commits (keep that up, some people on the project are unfamiliar with this practice and will ask you to squash and it is ok) instead of force pushes (much easier to review), but we try to only merge working versions without failing tests to master.

[v-jizhang]

Please squash the commits. I do appreciate changes during review being represented as commits (keep that up, some people on the project are unfamiliar with this practice and will ask you to squash and it is ok) instead of force pushes (much easier to review), but we try to only merge working versions without failing tests to master.

Got a message from squash "(nothing to squash)Already up to date." That because I force pushed.

[aweisberg]

I still see two commits in the PR. It should be possible to git log to get a hash and then git rebase -i <hash> to squash commits together and then force push.

[v-jizhang]

@aweisberg Squashed. Thanks

[aweisberg]

@pettyjamesm do you mind merging this?

[pettyjamesm]

The test implementation changes to fix the usages of @BeforeMethod LGTM. The usage of a regex rule to enforce it though does seem kind of brittle and has some performance risks, ask you pointed out.

It looks like Trino implemented a similar check in trinodb/trino#5412 and trinodb/trino#5420 using a test listener instead, which seems like a good compromise.

[v-jizhang]

The test implementation changes to fix the usages of @BeforeMethod LGTM. The usage of a regex rule to enforce it though does seem kind of brittle and has some performance risks, ask you pointed out.

It looks like Trino implemented a similar check in trinodb/trino#5412 and trinodb/trino#5420 using a test listener instead, which seems like a good compromise.

I'll take a look. Thanks.

[aweisberg]

Replaced by #15914