TestNG tests produce "Static configuration methods can cause unexpected behavior" warnings

[vlsi]

Expected behavior

The test suite should not produce "Static configuration methods can cause unexpected behavior" warnings.

It looks like the tests should be adjusted to avoid unexpected behavior.

Actual behavior

There is a significant number of "Static configuration methods can cause unexpected behavior" warnings in the test suite.

Test case sample

TestNG > Regression2 > test.invokedmethodlistener.InvokedMethodListenerTest > issue87_method_orderning_with_disable_test_class STANDARD_OUT
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.C.someMethod3()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.C.someMethod3()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.A.someMethod1()]. Static configuration methods can cause  unexpected behavior.
    [Configuration] [WARN] Detected a static method [test.invokedmethodlistener.C.someMethod3()]. Static configuration methods can cause  unexpected behavior.

[juherr]

Agree, except we need to keep one non-regression test for this unexpected use case.

Feel free to fix it ;)

[vlsi]

Yeah, it looks like all those warnings were introduced accidentally (copy-pasted from issue reproducers), and it is not clear if static was important or it was just a typo.

I wonder what if there was a flag like i.know.static.methods.can.cause.unexpected.behavior.please.silence.warning so the flag could be added to the relevant tests (e.g. the ones who print the most) to suppress the warnings.

[juherr]

In fact, I suppose the static support was a feature at the beginning because JUnit did it. But it is not the TestNG approach and that's why we added a warning. Current tests weren't updated :p

I like your idea about the flag for the tests using the static approach (when expected).

[vlsi]

So it might look like suppress:static_configuration,errors_in_configuration,... configuration feature so the users could suppress individual warnings.

[juherr]

I like the idea. WDYT @krmahadevan ?

[krmahadevan]

@juherr - You mean that we have a configuration parameter, which would be enabled by default, but which can be explicitly silenced ? Sure. I am fine with it. But the warning needs to show up by default (else there's no point in even having that warning in the first place and we might as well get rid of the warning).

[gribanoveu]

In the end, is it possible to turn off this warning?

[krmahadevan]

In the end, is it possible to turn off this warning?

@Eugene-grb - Its still not implemented. So NO its not possible to turn off this warning yet.

[vlsi]

@krmahadevan , well, the question was "does it still make sense to keep the warning?". I could easily create a PR that would just remove the warning. However, if the warning was important, then we should not just remove it.

---

Ok, I did some research, and here's the PR that added the warning: https://github.com/cbeust/testng/pull/2369#issuecomment-695747344

TL;DR is as follows:

• TestNG design is "reuse test instances"
• As there's only one instance of each test class, it does not make much difference when @BeforeClass (or even @BeforeSuite) is static or not. Ultimately, there's just a single instance, so it does not matter much if the configuration is static or not.

With JUnit, the approach is different:

• They prefer re-creating test instances for each test method
• That is why calling @BeforeAll on an instance method would make no sense for JUnit since it would discard the instance anyway

Frankly speaking, I am not sure which way is better (I believe both frameworks allow users to specify keep/recreate test instances). However, there might be cases when BeforeSuite does not really access instance methods (e.g. it just prints a message to a log or something). In that case, marking @BeforeSuite method static in Java makes it super-clear for the reader that the method does not access/modify instance fields.

---

I would suggest:

1) Consider static @BeforeSuite and @AfterSuite a non-issue (it aligns with https://github.com/cbeust/testng/issues/2294#issuecomment-614192473 ) 2) Reword "Static configuration methods can cause unexpected behavior" as something like "@BeforeClass/AfterClass is executed on a test instance before executing all the methods, so if you have multiple test instances a static configuration method might overwrite each other results. Consider a static @BeforeSuite/@AfterSuite if you need a global configuration or make the method non-static"