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.

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

Jump to bottom

Use ConTester to prove that the synchronized block is required #4672

Merged
merged 1 commit into from May 29, 2022
Merged

Use ConTester to prove that the synchronized block is required #4672

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions detekt-parser/build.gradle.kts
View file
Open in desktop
Expand Up @@ -7,6 +7,8 @@ plugins {
dependencies {
api(libs.kotlin.compilerEmbeddable)
implementation(projects.detektPsiUtils)
implementation(libs.contester.breakpoint)
testImplementation(libs.contester.driver)
testImplementation(projects.detektTestUtils)
testImplementation(libs.assertj)
}
Expand Down
4 changes: 4 additions & 0 deletions detekt-parser/src/main/kotlin/io/github/detekt/parser/DetektPomModel.kt
View file
Open in desktop
@@ -1,5 +1,6 @@
package io.github.detekt.parser

import io.github.davidburstrom.contester.ConTesterBreakpoint
import org.jetbrains.kotlin.com.intellij.openapi.extensions.ExtensionPoint
import org.jetbrains.kotlin.com.intellij.openapi.extensions.Extensions.getRootArea
import org.jetbrains.kotlin.com.intellij.openapi.project.Project
Expand All @@ -26,6 +27,9 @@ class DetektPomModel(project: Project) : UserDataHolderBase(), PomModel {
// Addresses https://github.com/detekt/detekt/issues/4609
synchronized(extensionArea) {
if (!extensionArea.hasExtensionPoint(extension)) {
ConTesterBreakpoint.defineBreakpoint("DetektPomModel.registerExtensionPoint") {
extensionArea == getRootArea()
Copy link
Member

Choose a reason for hiding this comment

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

could you put a named parameter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you provide an example of what you'd like it to look like?

Copy link
Member

Choose a reason for hiding this comment

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

ConTesterBreakpoint.defineBreakpoint(
    name = "DetektPomModel.registerExtensionPoint"
    onlyWhen = { extensionArea == getRootArea() }
)

Something along those lines. onlyWhen could be when or condition or whatever you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. That's not possible unless there are Kotlin bindings for the library. condition is preferable, to mimic debugger terminology.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry. I naively thought you wrote this in Kotlin, my bad :) You could add bindings for that btw.

condition is preferable, to mimic debugger terminology.

Yup that sounds great to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I would not want to impose a Kotlin runtime dependency for such a specific tool :)

}
extensionArea.registerExtensionPoint(
extension,
extensionClass,
Expand Down
15 changes: 15 additions & 0 deletions detekt-parser/src/test/kotlin/io/github/detekt/parser/KtCompilerSpec.kt
View file
Open in desktop
@@ -1,5 +1,6 @@
package io.github.detekt.parser

import io.github.davidburstrom.contester.ConTesterDriver
import io.github.detekt.psi.BASE_PATH
import io.github.detekt.psi.LINE_SEPARATOR
import io.github.detekt.psi.RELATIVE_PATH
Expand All @@ -8,10 +9,24 @@ import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatIllegalArgumentException
import org.jetbrains.kotlin.com.intellij.psi.PsiErrorElement
import org.jetbrains.kotlin.psi.KtTreeVisitorVoid
import org.junit.jupiter.api.AfterEach
import org.junit.jupiter.api.Nested
import org.junit.jupiter.api.Test

class KtCompilerSpec {
@AfterEach
internal fun tearDown() {
ConTesterDriver.cleanUp()
}

@Test
fun `parallel construction of KtCompilers should be thread safe`() {
val thread1 = ConTesterDriver.thread { KtCompiler() }
val thread2 = ConTesterDriver.thread { KtCompiler() }
ConTesterDriver.runToBreakpoint(thread1, "DetektPomModel.registerExtensionPoint")
ConTesterDriver.runUntilBlockedOrTerminated(thread2)
ConTesterDriver.join(thread1)
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

nit: That's mostly an API design decision, but they would read better if they're extension function instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what you're saying, though I'm not too keen on extension functions, especially since they would be called join(), start() etc which already overload and would be confusing. How about some sort of ConTester context, so the nature of the test is obvious?

Copy link
Member

Choose a reason for hiding this comment

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

especially since they would be called join(), start() etc which already overload and would be confusing.

Agree. If you end up this way, I would not use those names and be a bit more specific with function naming.

How about some sort of ConTester context, so the nature of the test is obvious?

That also works eventually :)

}

@Nested
inner class `Kotlin Compiler` {
Expand Down
3 changes: 3 additions & 0 deletions gradle/libs.versions.toml
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ jacoco = "0.8.8"
kotlin = "1.6.21"
ktlint = "0.45.2"
junit = "5.8.2"
contester = "0.2.0"

[libraries]
githubRelease-gradle = "com.github.breadmoirai:github-release:2.3.7"
Expand Down Expand Up @@ -37,6 +38,8 @@ reflections = "org.reflections:reflections:0.10.2"
mockk = "io.mockk:mockk:1.12.4"
snakeyaml = "org.yaml:snakeyaml:1.30"
jcommander = "com.beust:jcommander:1.82"
contester-breakpoint = { module = "io.github.davidburstrom.contester:contester-breakpoint", version.ref = "contester" }
contester-driver = { module = "io.github.davidburstrom.contester:contester-driver", version.ref = "contester" }

[plugins]
binaryCompatibilityValidator = { id = "org.jetbrains.kotlinx.binary-compatibility-validator", version = "0.10.0" }
Expand Down