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

Instantiate testClass taking into account its enclosing class #466

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

ivandev0
Copy link
Collaborator

@ivandev0 ivandev0 commented Feb 3, 2025

No description provided.

@ndkoval
Copy link
Collaborator

ndkoval commented Feb 3, 2025

@ivandev0, please create a separate PR for the migration to JUnit 5. Also, let's keep using imports with * whenever possible.

@ndkoval ndkoval requested a review from eupp February 4, 2025 10:59
@eupp
Copy link
Collaborator

eupp commented Feb 5, 2025

Let's maybe drop JUnit 5 commit from this PR for now?

It looks like migration to JUnit 5 might take some time due to the problem mentioned in #467 (comment)

@ivandev0
Copy link
Collaborator Author

ivandev0 commented Feb 5, 2025

Unfortunately, the migration is required because I am using Nested annotation to be able to run nested test. I didn't find an option to run nested tests with JUnit4.

@eupp
Copy link
Collaborator

eupp commented Feb 5, 2025

Lincheck does not force you to use JUnit test classes. You can rewrite the test to achieve the same goal:

class OuterClass {
    inner class InnerClass {
        @Operation
        fun zero() = 0
    }
}

class InnerClassTest {
    @Test
    fun someTest() {
        StressOptions()
            .invocationsPerIteration(1)
            .iterations(1)
            .check(OuterClass.InnerClass::class)
    }
}

BTW, I renamed the test to InnerClassTest to better reflect its semantics.

@ivandev0 ivandev0 marked this pull request as ready for review February 6, 2025 12:48
return constructor.newInstance()
}

if (this.enclosingClass != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we check if enclosingClass != null as a first step, and only then look for default constructor with zero arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. Class can have a default constructor even if enclosingClass != null. For example

package org.example

class A {
    inner class B
}

class C {
    class D
}

fun main() {
    println(A.B::class.java.enclosingClass) // class org.example.A
    println(C.D::class.java.enclosingClass) // class org.example.C
    // But
    A().B() // requires A object
    C.D()   // doesn't require C object
}

I can probably add such a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see, thanks.

Yes, it would be nice to add another test case for nested non-inner class (static nested in Java terminology).

@@ -329,3 +329,20 @@ internal inline fun <R> runOutsideIgnoredSection(descriptor: ThreadDescriptor?,

internal const val LINCHECK_PACKAGE_NAME = "org.jetbrains.kotlinx.lincheck."
internal const val LINCHECK_RUNNER_PACKAGE_NAME = "org.jetbrains.kotlinx.lincheck.runner."

internal fun <T> Class<T>.newInstanceRecursive(): T {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name newInstanceRecursive is a bit misleading IMO.
We just want to create some "default" instance of the class. We do not really care if it is inner or not, it is mostly an implementation detail.

So maybe rename to something like newDefaultInstance() ?

@ivandev0 ivandev0 requested a review from eupp February 6, 2025 17:59
@eupp eupp merged commit 5732aed into develop Feb 6, 2025
20 checks passed
@eupp eupp deleted the kylchik/develop branch February 6, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants