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

Embedding springmockk 4.0.2 into mockk #1218

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Embedding springmockk 4.0.2 into mockk #1218

wants to merge 5 commits into from

Conversation

oleksiyp
Copy link
Collaborator

@oleksiyp oleksiyp commented Feb 11, 2024

Based on the conversation Ninja-Squad/springmockk#112 we would like to embed springmockk as one of the mockk modules.

Copy link
Contributor

@kkurczewski kkurczewski left a comment

Choose a reason for hiding this comment

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

So I ran test on repository which uses:

  • Java 17
  • Kotlin 1.8.21
  • SpringMockk 4.0.2

I swapped artifact from maven central to:

  1. local JAR from NinjaSquad repo before my fork
  2. my fork of NinjaSquad repo after swapping packages
  3. local JAR from springmockk module from this branch

First and second attempt passed while third failed with:

        Caused by:
        java.lang.IllegalStateException: Error processing condition on org.springframework.boot.autoconfigure.transaction.TransactionAutoConfiguration$EnableTransactionManagementConfiguration
            at org.springframework.boot.autoconfigure.condition.SpringBootCondition.matches(SpringBootCondition.java:60)
            at org.springframework.context.annotation.ConditionEvaluator.shouldSkip(ConditionEvaluator.java:108)
            at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader$TrackedConditionEvaluator.shouldSkip(ConfigurationClassBeanDefinitionReader.java:470)
            at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader$TrackedConditionEvaluator.shouldSkip(ConfigurationClassBeanDefinitionReader.java:459)
            at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader$TrackedConditionEvaluator.shouldSkip(ConfigurationClassBeanDefinitionReader.java:459)
            at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitionsForConfigurationClass(ConfigurationClassBeanDefinitionReader.java:131)
            at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitions(ConfigurationClassBeanDefinitionReader.java:120)
            at org.springframework.context.annotation.ConfigurationClassPostProcessor.processConfigBeanDefinitions(ConfigurationClassPostProcessor.java:428)
            at org.springframework.context.annotation.ConfigurationClassPostProcessor.postProcessBeanDefinitionRegistry(ConfigurationClassPostProcessor.java:289)
            at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanDefinitionRegistryPostProcessors(PostProcessorRegistrationDelegate.java:349)
            at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:118)
            at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:788)
            at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:606)
            at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:754)
            at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:456)
            at org.springframework.boot.SpringApplication.run(SpringApplication.java:334)
            at org.springframework.boot.test.context.SpringBootContextLoader.lambda$loadContext$3(SpringBootContextLoader.java:137)
            at org.springframework.util.function.ThrowingSupplier.get(ThrowingSupplier.java:58)
            at org.springframework.util.function.ThrowingSupplier.get(ThrowingSupplier.java:46)
            at org.springframework.boot.SpringApplication.withHook(SpringApplication.java:1454)
            at org.springframework.boot.test.context.SpringBootContextLoader$ContextLoaderHook.run(SpringBootContextLoader.java:552)
            at org.springframework.boot.test.context.SpringBootContextLoader.loadContext(SpringBootContextLoader.java:137)
            at org.springframework.boot.test.context.SpringBootContextLoader.loadContext(SpringBootContextLoader.java:108)
            at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContextInternal(DefaultCacheAwareContextLoaderDelegate.java:225)
            at org.springframework.test.context.cache.DefaultCacheAwareContextLoaderDelegate.loadContext(DefaultCacheAwareContextLoaderDelegate.java:152)
            ... 81 more

            Caused by:
            java.lang.IllegalArgumentException: Invalid value type for attribute 'factoryBeanObjectType': java.lang.String

So something was broken during migration into mockk module.

Comment on lines +1 to +8
# Inclusion of springmockk into mockk

It was agreed that further maintenance of springmockk is done by mockk community
and project springmockk codebase is included in mockk.

Codebase is based on version springmockk 4.0.2 dcbe643 and tuned by
kkurczewski in https://github.com/kkurczewski/springmockk/tree/migration

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this note make more sense in commit message. Let say a year from now it won't be that much important for readers to include it in very top of README.

// But it would retain references to mocks that aren't used anymore (because the Spring context cache
// has a limit on the number of cached contexts). See https://github.com/Ninja-Squad/springmockk/issues/97
// A weak hashmap would be ideal, but it uses equals and hashCode, which would cause hashCode() to be called on the mocks,
// and confirmVerified calls to fail. See https://github.com/Ninja-Squad/springmockk/issues/27
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI code has references to original repository. If this repo will be removed then we will have dangling references.

Maybe this isn't a big deal but it is something we should keep in mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issues and original repo shouldnt be deleted, but archived

Copy link
Contributor

Choose a reason for hiding this comment

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

We should also update root README as it points to old repository.

Specifically I mean this line:
https://github.com/mockk/mockk/blob/master/README.md?plain=1#L88

Comment on lines +38 to +40
implementation(Deps.Libs.springBootTest)
implementation(Deps.Libs.springTest)
implementation(Deps.Libs.springContext)
Copy link
Contributor

Choose a reason for hiding this comment

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

I did further tests, dependency which break springmockk is: org.springframework:spring-context:6.1.0 and above

Bumping Spring caused dependency mismatch. Following code works:

testImplementation("io.mockk:springmockk:1.13.10-SNAPSHOT") {
    exclude group: 'org.springframework.boot'
    exclude group: 'org.springframework'
}
//testImplementation 'org.springframework:spring-context:6.1.0' // this version breaks most recent version of springmockk

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This happens due to some internal type mismatches in factoryBeanObjectType. This looks like some internal interface in spring which is not standardized and changes between versions. I will try to mitigate this issue somehow.

@ajax-tupis-r
Copy link

@oleksiyp when can we expect this PR to be merged?

@Raibaz
Copy link
Collaborator

Raibaz commented Apr 5, 2024

I think there are still a few open points to be addressed before this can be merged.

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.

None yet

4 participants