Skip to content

Extract utbot-spring-framework module from utbot-framework #2570

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

Merged
merged 9 commits into from
Sep 7, 2023

Conversation

IlyaMuravjov
Copy link
Collaborator

Description

All spring-specific logic is extracted from utbot-framework module to utbot-spring-framework module.

How to test

Regression tests, this PR should not change UtBot semantics at all.

Self-check list

  • I've set the proper labels for my PR (at least, for category and component).
  • PR title and description are clear and intelligible.
  • I've added enough comments to my code, particularly in hard-to-understand areas.
  • The functionality I've repaired, changed or added is covered with automated tests.
  • Manual tests have been provided optionally.
  • The documentation for the functionality I've been working on is up-to-date.

@IlyaMuravjov IlyaMuravjov added ctg-refactoring Issue related to refactoring process comp-spring Issue is related to Spring projects support labels Aug 30, 2023
Copy link
Member

@Damtev Damtev left a comment

Choose a reason for hiding this comment

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

There are some unobvious moments

Comment on lines +251 to +255
watchdog.measureTimeForActiveCall(perform, "Performing dynamic task") { params ->
val task = kryoHelper.readObject<EngineProcessTask<Any?>>(params.engineProcessTask)
val result = task.perform(kryoHelper)
kryoHelper.writeObject(result)
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have only one post-processing task? Perhaps it could be a list of such tasks performed consequently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. It's not a post-processing task, the order in witch callbacks are registered in the engine process has nothing to do with the order in which they are called by the IDE process. For example, callback for findMethodsInClassMatchingSelected is registered after the callback for generate, despite generate being called after findMethodsInClassMatchingSelected.
  2. If we need to perform multiple tasks we can call perform multiple times, the same way generate is called multiple times when we generate tests for multiple classes.

@@ -0,0 +1,39 @@
val kotlinLoggingVersion: String by rootProject
val rdVersion: String by rootProject
val sootVersion: String by rootProject
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need Soot for this module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now NonNullSpeculator accepts SootField.

I've tried making engine convert SootField to FieldId before passing it to speculativelyCannotProduceNullPointerException, but it was failing to convert declaring SootClass to a valid ClassId for some lambdas, causing ClassNotFoundException when the default implementation of the speculativelyCannotProduceNullPointerException tried to retrieve the packageName which in turned used jClass.

@@ -0,0 +1,27 @@
package org.utbot.framework.codegen.domain

abstract class SpringModule(
Copy link
Member

Choose a reason for hiding this comment

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

Why not make it an enum?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Made it a enum.

import org.utbot.rd.use
import org.utbot.spring.process.SpringAnalyzerProcess

class SpringAnalyzerTask(
Copy link
Member

Choose a reason for hiding this comment

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

Is it true that it does not matter will this task be performed before the symbolic analysis or after? So, can we always consider it as a post-processing task?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's always performed before symbolic analysis and its results are later used to guide type replacement during symbolic execution.

Comment on lines +12 to +20
object RelevantFieldManagersProperty : CgContextProperty<MutableList<CgAbstractClassFieldManager>>

/**
* Managers to process annotated fields of the class under test
* relevant for the current generation type.
*/
val CgContextOwner.relevantFieldManagers: MutableList<CgAbstractClassFieldManager>
get() = properties.getOrPut(RelevantFieldManagersProperty) { mutableListOf() }

Copy link
Member

Choose a reason for hiding this comment

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

It's not very obvious why this object and this extension method (which seem to be common for any possible extension to the CgContext - Spring, or any other in the future) are located in this Spring-specific module

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, there's nothing Spring-specific in declaring class fields. However, right now adding FieldManagers won't have any effect unless CgSpringVariableConstructor is used. I've been always in favor of separating Spring-specific and field-declaration-specific logic into 2 decorators of VariableConstructor instead of one inheritor made an issue regarding that, see #2579.

val springAnalyzerResult = structdef {
field("beanDefinitions", array(beanDefinitionData))
val performParams = structdef {
field("engineProcessTask", array(PredefinedType.byte))
Copy link
Member

Choose a reason for hiding this comment

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

Based on its usage, this task seems to always be post-processing. Am I right? If so, please rename it correspondingly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, see reply to the first comment.

@IlyaMuravjov IlyaMuravjov requested a review from Damtev September 6, 2023 14:30
Copy link
Member

@Damtev Damtev left a comment

Choose a reason for hiding this comment

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

LGTM

@IlyaMuravjov IlyaMuravjov merged commit ed7d870 into main Sep 7, 2023
@IlyaMuravjov IlyaMuravjov deleted the ilya_m/spring-module branch September 7, 2023 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-spring Issue is related to Spring projects support ctg-refactoring Issue related to refactoring process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants