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

Feature/add di mock chek #24

Merged
merged 9 commits into from
Mar 14, 2024

Conversation

huangliang992
Copy link
Contributor

mock fields according to the dependency injection annotations of class and fields, if a class has annotation like @service, @controller, @component ..., mock the filed with annotation @Autowired or @resource

@yaronyam
Copy link
Member

yaronyam commented Mar 9, 2024

Hats off for the initiative & effort 🙏 however, I have some concerns for cases where mock will not be generated where it should and regarding impact on performance (extending the traversed object tree). Before I raise technical comments, need to decide we should continue with this effort. It depends on the motivation, which problems are we trying to solve, what effort we're willing to invest and what is the potential negative impact (performance, complexity, backward compatibility).
The main issues with this PR - it's not enough to check annotations on a PsiField to deduct that it's under DI. Field can be impacted by:

  • annotation on a setter
  • annotation on constructor
  • may be initialized by different block - i.e. in spring @PostConstruct
  • DI may be defined in xml. though that's a rarely used edge case to have class level annotations mixed with xml DI definitions for the same class.

Other issues - that are less concerning (lower impact or easier to tackle):

  • likewise it's not enough to read class level annotations to deduct that class is under DI (annotations can be found on c'tor, xml definitions etc..)
  • need to generalize to support other DI SDK/Frameworks (now or later) - JSR-330, Guice, Dagger
  • need to optimize performance (integration tests currently don't reflect potential performance issues due to psi tree traversal imported libs (Spring etc.)

We can tackle some of these issue, but not sure the effort and resulted complexity would pay off, if the main use case it to avoid mocking fields such loggers (where those are not initialized by constructor arguments/setters).
for loggers and alike, we can avoid mocking if:

  1. initialized inline (for loggers these would typically be static fields)
  2. initialized in static block
  3. initialized in ctor - but their value is not passed as ctor arg

use case 1 - is relevant for most implementations - that can be tackled easily with relatively lower impact on existing functionality:
either add Field#hasInitializer -> com.intellij.psi.impl.source.PsiFieldImpl#hasInitializer - and use it to avoid generation mock when set. alternatively avoid adding that Field to Type (a potential breaking change)- in case we're sure it won't be useful/relevant in any case for test code generation. one potential pitfall I can think of - setting a default value that is overridden when class initialized (by ctor, DI) - where u'd like to control it's behavior with a mock

use cases 2,3 - are more complex, but used less often, can be supported later when prioritized, by calling psiField.getContainingClass().getInitializers() - search in nested child expressions of each (there's some complexity when private methods called...)

@huangliang992
Copy link
Contributor Author

What I am considering is the general Spring Di scenario. According to the Spring convention, which is greater than the common way of writing configurations. Would it be more accurate that we mock field with annotations such as @autowire in classes with annotations such as @service or @component. And it won't effect other scenarios.

Alternatively, can we allow users to choose the objects they need to mock and the testing methods through pop-up boxes. I found that other plugins do it this way, like SquareTest。
Dingtalk_20240311100241

@yaronyam
Copy link
Member

Would be great to support custom settings on demand when generating test. would also like to add other settings for selection. But anyway, default settings should be best suited for most use cases without going through additional setting form.

I think following logic would avoid negative impact.
Avoid mocking a field if all these conditions are met:

  1. field has no direct DI annotation
  2. field does not have a setter
  3. class has DI annotation
  4. there is no class ctor with DI annotation.
  • later, logic can be added to resolve if field in under DI when set via ctor annotation. when that happens, condition 4 noted above can be removed.
  • to check if field & class are under DI need to match at least Spring and JSR-330 SDK annotation types since they can be used interchangeably.
  • instead of adding List<Type> annotations; field to Type/Field class, I think better settle with boolean isAnnotatedByDI field, to reduce extending object tree at this stage.

@huangliang992
Copy link
Contributor Author

Okay, I'll think about how to handle this next

@huangliang992
Copy link
Contributor Author

can condition 4 be replaced with "there is no class ctor"?

@yaronyam
Copy link
Member

yes, 4th condition can be widen as suggested

@huangliang992
Copy link
Contributor Author

I updated the code and added some itegration tests for the scenes

Copy link
Member

@yaronyam yaronyam left a comment

Choose a reason for hiding this comment

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

great to see additional test scenarios being covered. looks great 👍 posted some technical comments

@yaronyam yaronyam merged commit bc6b95d into wrdv:master Mar 14, 2024
5 checks passed
@yaronyam
Copy link
Member

Released in version 6.3

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.

2 participants