Add checkerframework and errorprone #2941

juherr · 2023-07-31T06:57:34Z

Try checkerframework and errorprone

juherr · 2023-07-31T07:01:10Z

@vlsi Any feedback about the 2 tools and how I added them in the build (even if I was inspired by what you did on pg-driver)?

@krmahadevan WDYT? They highlight a lot of minor issues but some potential problems too.

krmahadevan · 2023-07-31T07:42:33Z

@juherr - This is definitely a good addition. I am yet to explore both these tools. I have already seen error prone being used by jhipster when it scaffolds a new project based on my inputs.

Couple of questions:

These highlight the problems in our code, but would it prevent TestNG from being used incorrectly by its consumers (especially the ones that use the TestNG apis to provide customised runners etc., ?)
The CI has failed, mainly because of the discrepancies in our code, which you also highlighted. So for us to adopt these two tools, I believe we would need to iterate on all the failures and then fix them right ?

juherr · 2023-07-31T08:59:02Z

Thanks.

These highlight the problems in our code, but would it prevent TestNG from being used incorrectly by its consumers (especially the ones that use the TestNG apis to provide customised runners etc., ?)

As I understand, yes. It provides extra data in bytecode thanks to the annotations.
The drawback is it is adding a new runtime dependency (because the annotations are stored in the bytecode).

The CI has failed, mainly because of the discrepancies in our code, which you also highlighted. So for us to adopt these two tools, I believe we would need to iterate on all the failures and then fix them right ?

To be honest, I hoped there were a bit less. That's why I sent the draft before fixing them all.

vlsi · 2023-07-31T13:03:59Z

"nullability" is not trivial, so it might take some time to properly annotate and identify nullable types.
The general advice is assume "everything is not nullable by default" (==almost never put @NotNull), and annotate @Nullable where null values are allowed.

vlsi · 2023-07-31T13:05:03Z

build-logic/code-quality/src/main/kotlin/testng.checkerframework.gradle.kts

+plugins.withId("org.checkerframework") {
+    configure<CheckerFrameworkExtension> {


plugins { id("org.checkerframework") } adds checkerframework plugin, so plugins.withId(..) is not needed here.

Something like checkerframework {...} should do.

vlsi · 2023-07-31T13:07:15Z

testng-asserts/src/main/java/org/testng/Assert.java

   */
-  public static void assertEquals(Object[] actual, Object[] expected, String message) {
+  public static void assertEquals(
+      @Nullable Object[] actual, @Nullable Object[] expected, @Nullable String message) {


I believe it should be @Nullable Object @Nullable [] actual: nullable array of nullable elements

vlsi · 2023-07-31T13:09:50Z

testng-core-api/src/main/java/org/testng/IObjectFactory2.java

   */
  @Deprecated
-  default Object newInstance(Class<?> cls) {
+  default Object newInstance(@NonNull Class<?> cls) {


It would be better to use package-level "default not null" annotation rather than @NotNull: https://github.com/pgjdbc/pgjdbc/blob/aff581fe806f1c1cdc8bcd9e5a981e37691b44b5/pgjdbc/src/main/java/org/postgresql/package-info.java#L6-L8

See explanation in https://checkerframework.org/manual/#climb-to-top

vlsi · 2023-07-31T13:15:05Z

In my experience, checkerframework nullability verification takes time, so I tend to activate it only when -PenableCheckerframework parameter is specified. Then checkerframework executes in a single CI job only.

See https://github.com/apache/jmeter/blob/c94db9741f0547235f9a67b1ece40389add6b742/build-logic/build-parameters/build.gradle.kts#L77 , and https://github.com/apache/jmeter/blob/c94db9741f0547235f9a67b1ece40389add6b742/build-logic/verification/src/main/kotlin/build-logic.style.gradle.kts#L42-L43 (conditional activation of .checkerframework.gradle.kts)

juherr · 2023-07-31T14:45:09Z

Thanks for the feedbacks @vlsi ❤️

juherr · 2023-08-07T07:05:37Z

Updated according to reviews.

All issues are not fixed yet.

Still 2 gradle issues: not succeeding in using the parameters plugin + not finding the way to merge package-info

vlsi · 2023-08-08T07:05:23Z

testng-asserts/src/main/java/org/testng/Assert.java

   * @param message the assertion error message
   */
-  public static void assertEquals(Object actual, Object expected, String message) {
+  public static void assertEquals(Object actual, Object expected, @Nullable String message) {


Suggested change

public static void assertEquals(Object actual, Object expected, @Nullable String message) {

public static void assertEquals(@Nullable Object actual, @Nullable Object expected, @Nullable String message) {

vlsi · 2023-08-08T07:05:42Z

testng-asserts/src/main/java/org/testng/Assert.java

   * @param message the assertion error message
   */
-  public static void assertEquals(Double actual, Double expected, String message) {
+  public static void assertEquals(Double actual, Double expected, @Nullable String message) {


Suggested change

public static void assertEquals(Double actual, Double expected, @Nullable String message) {

public static void assertEquals(@Nullable Double actual, @Nullable Double expected, @Nullable String message) {

vlsi · 2023-08-08T07:05:53Z

testng-asserts/src/main/java/org/testng/Assert.java

   * @param message the assertion error message
   */
-  public static void assertEquals(Float actual, Float expected, String message) {
+  public static void assertEquals(Float actual, Float expected, @Nullable String message) {


Suggested change

public static void assertEquals(Float actual, Float expected, @Nullable String message) {

public static void assertEquals(@Nullable Float actual, @Nullable Float expected, @Nullable String message) {

vlsi · 2023-08-08T07:06:37Z

testng-asserts/src/main/java/org/testng/Assert.java

   * @param expected the expected value
   */
-  public static void assertEquals(Iterator<?> actual, Iterator<?> expected) {
+  public static void assertEquals(@Nullable Iterator<?> actual, @Nullable Iterator<?> expected) {


Suggested change

public static void assertEquals(@Nullable Iterator<?> actual, @Nullable Iterator<?> expected) {

public static void assertEquals(@Nullable Iterator<@Nullable ?> actual, @Nullable Iterator<@Nullable ?> expected) {

vlsi · 2023-08-08T07:09:47Z

testng-core-api/src/main/java/org/testng/ITestResult.java

   */
  static boolean wasFailureDueToTimeout(ITestResult result) {
-    Throwable cause = result.getThrowable();
+    @Nullable Throwable cause = result.getThrowable();


Technically speaking, @Nullable should be automatically inferred for local variables, so this could probably be omitted

Suggested change

@Nullable Throwable cause = result.getThrowable();

Throwable cause = result.getThrowable();

I suppose you're right.

It means I should not use TypeUseLocation.ALL in @DefaultQualifier(value = NonNull.class, locations = TypeUseLocation.ALL)
Any other option I should exclude? https://checkerframework.org/api/org/checkerframework/framework/qual/TypeUseLocation.html

vlsi · 2023-08-08T07:10:32Z

testng-core-api/src/main/java/org/testng/internal/ClassHelper.java

  static List<ClassLoader> appendContextualClassLoaders(List<ClassLoader> currentLoaders) {
    List<ClassLoader> allClassLoaders = Lists.newArrayList();
-    ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
+    @Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();


Suggested change

@Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();

ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();

?

vlsi · 2023-08-08T07:10:54Z

testng-core-api/src/main/java/org/testng/internal/ClassHelper.java

-          } else {
-            m.addAll(extractedMethod.getValue());
-          }
+    @Nullable Class<?> parent = clazz.getSuperclass();


Suggested change

@Nullable Class<?> parent = clazz.getSuperclass();

Class<?> parent = clazz.getSuperclass();

vlsi · 2023-08-08T07:11:56Z

testng-core-api/src/main/java/org/testng/internal/ConstructorOrMethod.java


  public ConstructorOrMethod(Method m) {
    m_method = m;
+    m_constructor = null;


Default value is null anyway, so does it make sense to assign it with null?

vlsi · 2023-08-08T07:16:21Z

I would suggest merging errorprone first as it would require much fewer changes.

juherr · 2023-08-08T15:43:26Z

@vlsi Thanks for the review. I think your suggestion is a good strategy and I will make another PR with errorprone only.

Any idea why I'm not able to use build-parameters? The generated plugin is not found here https://github.com/testng-team/testng/pull/2941/files#diff-5bb00f6e0b0be0ec62ea3824c3888e27921683447cea9fb16bdd9f859f7881acR5

juherr added 2 commits July 31, 2023 08:54

Add checkerframework and errorprone

4822292

WIP Fix error from checkerframework and errorprone

a566d2c

vlsi reviewed Jul 31, 2023

View reviewed changes

juherr added 2 commits August 6, 2023 11:32

WIP Fix error from checkerframework and errorprone

b96fab6

WIP Fix error from checkerframework and errorprone

d6fbea3

juherr requested a review from vlsi August 7, 2023 07:06

WIP Fix error from checkerframework and errorprone

0c765e8

vlsi reviewed Aug 8, 2023

View reviewed changes

juherr mentioned this pull request Aug 15, 2023

Add errorprone #2958

Merged

		plugins.withId("org.checkerframework") {
		configure<CheckerFrameworkExtension> {

	public static void assertEquals(Object actual, Object expected, @Nullable String message) {
	public static void assertEquals(@Nullable Object actual, @Nullable Object expected, @Nullable String message) {

	public static void assertEquals(Double actual, Double expected, @Nullable String message) {
	public static void assertEquals(@Nullable Double actual, @Nullable Double expected, @Nullable String message) {

	public static void assertEquals(Float actual, Float expected, @Nullable String message) {
	public static void assertEquals(@Nullable Float actual, @Nullable Float expected, @Nullable String message) {

	@Nullable Throwable cause = result.getThrowable();
	Throwable cause = result.getThrowable();

	@Nullable ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
	ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();

	@Nullable Class<?> parent = clazz.getSuperclass();
	Class<?> parent = clazz.getSuperclass();

Add checkerframework and errorprone #2941

Are you sure you want to change the base?

Add checkerframework and errorprone #2941

Uh oh!

Conversation

juherr commented Jul 31, 2023

Uh oh!

juherr commented Jul 31, 2023

Uh oh!

krmahadevan commented Jul 31, 2023

Uh oh!

juherr commented Jul 31, 2023

Uh oh!

vlsi commented Jul 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlsi commented Jul 31, 2023

Uh oh!

juherr commented Jul 31, 2023

Uh oh!

juherr commented Aug 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vlsi commented Aug 8, 2023

Uh oh!

juherr commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants