Skip to content

Improves non mockability reasons also report mock maker, mockito version#3535

Open
bric3 wants to merge 8 commits into
mainfrom
adds-mock-maker-name-2
Open

Improves non mockability reasons also report mock maker, mockito version#3535
bric3 wants to merge 8 commits into
mainfrom
adds-mock-maker-name-2

Conversation

@bric3

@bric3 bric3 commented Dec 9, 2024

Copy link
Copy Markdown
Member

Improves a bit the error report

  • shows mock mack in more cases
  • friendly report of abstract sealed class being non mockable

Should help diagnosing in cases like

Supersedes #3531 as its gha workflow doesn't start anymore

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@bric3 bric3 mentioned this pull request Dec 9, 2024
8 tasks
@codecov-commenter

codecov-commenter commented Dec 9, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.16981% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.73%. Comparing base (7c277ac) to head (d3b0964).
⚠️ Report is 120 commits behind head on main.

Files with missing lines Patch % Lines
...on/bytebuddy/InlineDelegateByteBuddyMockMaker.java 33.33% 5 Missing and 3 partials ⚠️
...al/creation/bytebuddy/SubclassInjectionLoader.java 20.00% 8 Missing ⚠️
...rnal/stubbing/defaultanswers/ReturnsDeepStubs.java 42.85% 3 Missing and 1 partial ⚠️
...to/internal/creation/proxy/InvokeDefaultProxy.java 0.00% 2 Missing ⚠️
...ockito/internal/creation/proxy/ProxyMockMaker.java 33.33% 2 Missing ⚠️
...creation/bytebuddy/SubclassByteBuddyMockMaker.java 50.00% 1 Missing ⚠️
...ckito/internal/creation/bytebuddy/TypeSupport.java 0.00% 1 Missing ⚠️
.../org/mockito/internal/util/MockitoInformation.java 66.66% 1 Missing ⚠️
...e/src/main/java/org/mockito/plugins/MockMaker.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3535      +/-   ##
============================================
- Coverage     85.60%   84.73%   -0.88%     
+ Complexity     2954     2946       -8     
============================================
  Files           341      342       +1     
  Lines          8976     9006      +30     
  Branches       1118     1122       +4     
============================================
- Hits           7684     7631      -53     
- Misses         1003     1075      +72     
- Partials        289      300      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bric3
bric3 force-pushed the adds-mock-maker-name-2 branch from fe3dcbf to 52e746c Compare December 9, 2024 13:03
}

tasks.withType<Checkstyle> {
exclude("org/mockito/internal/util/MockitoVersion.java")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ultra-nit: define constant value to use both here and in the task implementation


configure<SpotlessExtension> {
java {
targetExclude(relativePath(generateVersionClass.get().generatedVersionClassDir) + "/**")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: to avoid eager configuration, let's also define a constant for project.layout.buildDirectory.dir("generated/sources/version/java"). That way, we define it once and use it twice to configure:

val generatedVersionClassDirLocation = project.layout.buildDirectory.dir("generated/sources/version/java")

generateVersionClass.configure {
    generatedVersionClassDir(generatedVersionClassDirLocation)
}

configure<SpotlessExtension> {
    java {
        targetExclude(relativePath(generatedVersionClassDirLocation) + "/**")
    }
}

That also allows us to remove the .convention on the task property

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good observation!

return INSTRUMENTATION.isModifiableClass(type) && !EXCLUDES.contains(type);
return INSTRUMENTATION.isModifiableClass(type)
&& !EXCLUDES.contains(type)
&& !(Modifier.isAbstract(type.getModifiers())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: use TypeSupport.isAbstractAndSealed here (and below)

if (type.isEnum()) {
return "Sealed abstract enums can't be mocked. Since Java 15 abstract enums are declared sealed, which prevents mocking. You can still return an existing enum literal from a stubbed method call.";
} else {
return "Sealed interfaces or abstract classes can't be mocked. Interfaces cannot be instantiated and cannot be subclassed for mocking purposes. Instead of mocking a sealed interface or an abstract class, a non-abstract class can be mocked and used to represent the interface.";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: use join to make it multiple lines and a bit more readable.

@bric3 bric3 Dec 9, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If I didn't it's because this string will appear has single reason prefixed by - . Joining will land the next line on column 0.

This can be worked around though.

@TimvdLippe

Copy link
Copy Markdown
Contributor

Seems like the build logs were cut off, but the raw logs still had the full build. Here's the relevant scan with failing tests: https://scans.gradle.com/s/72gtfyapmryfw

@bric3
bric3 force-pushed the adds-mock-maker-name-2 branch from 024937c to 5eaf285 Compare December 9, 2024 18:24
@sathya-ac9

This comment was marked as off-topic.

@bric3
bric3 force-pushed the adds-mock-maker-name-2 branch 4 times, most recently from 31a03ef to 1cba555 Compare December 14, 2024 15:36
@TimvdLippe

Copy link
Copy Markdown
Contributor

Can you split up the commits in separate PRs so we can debug these issues in isolation? That way we are more likely to figure out which commit/change is the culprit for instability

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.

4 participants