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

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

Merged
merged 1 commit into from May 29, 2022

Conversation

davidburstrom
Copy link
Contributor

@davidburstrom davidburstrom commented Apr 4, 2022

Relates to PR #4631

The issue that was fixed was never proven, due to lack of test tooling. Therefore, I wrote the ConTester utility (https://github.com/davidburstrom/contester) which allows for defining breakpoints in the production code.

If the synchronized block is removed, the tests start to fail for the reason indicated in #4609

@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #4672 (4a0a216) into main (2f5b8f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #4672   +/-   ##
=========================================
  Coverage     84.80%   84.80%           
- Complexity     3452     3453    +1     
=========================================
  Files           492      492           
  Lines         11326    11328    +2     
  Branches       2086     2086           
=========================================
+ Hits           9605     9607    +2     
  Misses          673      673           
  Partials       1048     1048           
Impacted Files Coverage Δ
...n/kotlin/io/github/detekt/parser/DetektPomModel.kt 80.00% <100.00%> (+1.73%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f5b8f5...4a0a216. Read the comment docs.

Copy link
Member

@cortinico cortinico left a comment

That's awesome 👍 It would help us catch a lot of concurrency problems we have. As a side note: we probably want to wait for a stable release of Contester before merging and releasing this.

@@ -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

@cortinico cortinico Apr 6, 2022

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

@davidburstrom davidburstrom Apr 7, 2022

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

@cortinico cortinico Apr 7, 2022

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

@davidburstrom davidburstrom Apr 7, 2022

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

@cortinico cortinico Apr 7, 2022

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

@davidburstrom davidburstrom Apr 9, 2022

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 :)

ConTesterDriver.runToBreakpoint(thread1, "DetektPomModel.registerExtensionPoint")
ConTesterDriver.runUntilBlockedOrTerminated(thread2)
ConTesterDriver.join(thread1)
Copy link
Member

@cortinico cortinico Apr 6, 2022

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

@davidburstrom davidburstrom Apr 7, 2022

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

@cortinico cortinico Apr 7, 2022

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 :)

@davidburstrom
Copy link
Contributor Author

davidburstrom commented Apr 7, 2022

That's awesome 👍 It would help us catch a lot of concurrency problems we have. As a side note: we probably want to wait for a stable release of Contester before merging and releasing this.

Thanks :) It'd especially useful to regression test them.

As for the side note: Yeah, I've possibly been too defensive about the naming, wanting to get a little bit of feedback on the tool before going 0.1.0 proper. I'd say its quite release ready as it is.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Apr 7, 2022

It's odd to have test code in production code. But we already had regressions on this code so I imagine that it is worth it.

I assume that the overhead in production is near to 0, right?

@davidburstrom
Copy link
Contributor Author

davidburstrom commented Apr 7, 2022

The overhead is 0 for repeated calls, see https://github.com/davidburstrom/contester#benchmarks
Though, I should benchmark the first call too.

@davidburstrom
Copy link
Contributor Author

davidburstrom commented Apr 8, 2022

It's odd to have test code in production code. But we already had regressions on this code so I imagine that it is worth it.

I get what you mean. Though, it's not much different than usage of @VisibleForTesting annotations, and other production constructs that only exist to bolster testability. At least this one doesn't affect the API of the system under test.

I assume that the overhead in production is near to 0, right?

I've updated the benchmark, the first call is on micro-second duration.

@davidburstromspotify davidburstromspotify force-pushed the main-prove-pr-4631 branch 2 times, most recently from 8b8fcbf to 9572741 Compare Apr 9, 2022
@davidburstrom
Copy link
Contributor Author

davidburstrom commented Apr 9, 2022

I've released a 0.1.0 GA, just waiting for it to appear on Maven Central.

@davidburstrom davidburstrom marked this pull request as ready for review Apr 9, 2022
@cortinico cortinico added this to the 1.20.0 milestone Apr 9, 2022
@BraisGabin BraisGabin modified the milestones: 1.20.0, 1.21.0 Apr 11, 2022
@BraisGabin
Copy link
Collaborator

BraisGabin commented Apr 11, 2022

I vote to move this one to 1.21. We can merge it as soon as we release 1.20 but because it adds a really new library in production code and we released two RC already without it I think its safer and we lose nearly nothing by waiting.

@davidburstrom
Copy link
Contributor Author

davidburstrom commented Apr 12, 2022

I'm fine with that :)

@github-actions github-actions bot added build dependencies Pull requests that update a dependency file labels May 29, 2022
@davidburstrom
Copy link
Contributor Author

davidburstrom commented May 29, 2022

@BraisGabin @cortinico I've released version 0.2.0 of ConTester which fixes a minor bug. Since Detekt 1.20.0 is released, could we consider merging this? I've rebased on top of latest main.

@BraisGabin BraisGabin merged commit a8cde4f into detekt:main May 29, 2022
17 of 20 checks passed
@davidburstrom
Copy link
Contributor Author

davidburstrom commented May 29, 2022

Thanks for that! :)

@cortinico
Copy link
Member

cortinico commented May 29, 2022

@BraisGabin @cortinico I've released version 0.2.0 of ConTester which fixes a minor bug. Since Detekt 1.20.0 is released, could we consider merging this? I've rebased on top of latest main.

Thanks for doing this 🙏

@cortinico cortinico removed the blocked label May 29, 2022
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build dependencies Pull requests that update a dependency file notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants