Force mocking without mock framework installed fix #278

nikitavlaev · 2022-06-22T09:51:49Z

Description

Added mock listeners for engine to make it abstract from external events.
Added force mock listener.
Added url handler for urls like "#utbot/..." to launch configuration tasks from popups.
Added message with link to mock framework configuration to test reports if force mocking happened.

Fixes #190

Type of Change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Manual Scenario

Launched project without mockito installed.
Generated tests for Sort.quickSort without mocks -> mocks for Random were forced.
Checked that message suggesting mockito installation appeared in test report.
Verified that link in that message leads to the window for mockito configuration, and it installs correctly.
After installation, tests with force-mocks were generated correctly.

Checklist (remove irrelevant options):

The change followed the style guidelines of the UTBot project
Self-review of the code is passed
The change contains enough commentaries, particularly in hard-to-understand areas
New documentation is provided or existed one is altered
No new warnings
Tests that prove my change is effective
All tests pass locally with my changes

EgorkaKulikov · 2022-06-23T17:21:37Z

utbot-framework/src/main/kotlin/org/utbot/engine/Mocks.kt

 fun shouldMock(
+ type: RefType,
+ mockInfo: UtMockInfo,
+ ): Boolean = doShouldMock(type, mockInfo).also { mockListenerController.onShouldMock(strategy, it) }


Shouldn't we emit an event if and only if doShouldMock returns true?

EgorkaKulikov · 2022-06-23T17:22:38Z

utbot-framework/src/main/kotlin/org/utbot/engine/UtBotSymbolicEngine.kt

-import kotlinx.coroutines.currentCoroutineContext
-import kotlinx.coroutines.CancellationException
-import kotlinx.coroutines.Job
+import kotlinx.coroutines.*


Correct your Intellij Idea settings to avoid wildcard imports, please. In my situation it was reset after the update of IDE.

Check it in other files of this PR too, please

EgorkaKulikov · 2022-06-24T06:49:51Z

utbot-framework/src/main/kotlin/org/utbot/engine/UtBotSymbolicEngine.kt

 )
 }

+ fun attachMockListener(mockListener: MockListener) = mocker.mockListenerController.attach(mockListener)


Let's discuss why this code is a pat of engine, may be there is a very unclear relation here

EgorkaKulikov · 2022-06-24T06:51:14Z

utbot-framework/src/main/kotlin/org/utbot/engine/util/mockListeners/ForceMockListener.kt

+import org.utbot.engine.util.mockListeners.exceptions.ForceMockCancellationException
+
+/**
+ * Listener for mocker events in symbolic engine.


It is always better to mention entities in comments as named references like [UtSymbolicEngine]

EgorkaKulikov · 2022-06-24T07:02:13Z

utbot-framework/src/main/kotlin/org/utbot/engine/util/mockListeners/ForceMockListener.kt

+ * Supposed to be created only if Mockito is not installed.
+ */
+class ForceMockListener: MockListener {
+ var forceMockHappened = false


Let's avoid public setter on it

EgorkaKulikov · 2022-06-24T07:04:31Z

utbot-framework/src/main/kotlin/org/utbot/engine/util/mockListeners/ForceMockListener.kt

+ // If force mocking happened
+ if (shouldMock) {
+ // Cancel engine job
+ controller.job!!.cancel(ForceMockCancellationException())


I think that we would never like to crash here. Suggest to do like this
controller.job?.let { ForceMockCancellationException() }

EgorkaKulikov · 2022-06-24T07:58:41Z

utbot-framework/src/main/kotlin/org/utbot/engine/util/mockListeners/MockListenerController.kt

+ * Controller that allows to attach listeners to mocker in symbolic engine.
+ */
+class MockListenerController(private val controller: EngineController) {
+ val listeners = mutableListOf<MockListener>()


What other listeners except MockListener may be in MockListenerController?

EgorkaKulikov · 2022-06-24T08:03:29Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/generator/TestGenerator.kt

+ if (model.forceMockingHappened) {
+ val warningMessage = """
+ Warning: Some test cases were ignored, because no mocking framework is installed in the project.
+ Better results could be achieved by <a href="${TestReportUrlOpeningListener.prefix}${TestReportUrlOpeningListener.mockitoSuffix}">installing mocking framework</a>, e.g. Mockito.


I don't like "e. g. Mockito"
If user install another framework. it will not help him. More than that, mockito-core with the version startting from 3.. is required to him.

EgorkaKulikov · 2022-06-24T08:06:20Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/Notifications.kt

+ if (createMockFrameworkNotificationDialog("Configure mock framework") == Messages.YES) {
+ configureMockFramework(project!!, module!!)
+ }
+ } ?: error("Could not configure mock framework: null in module ")


We need module or project name here: these message in logs will be just scaring

EgorkaKulikov · 2022-06-24T08:07:25Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/Notifications.kt

+ private val defaultListener = NotificationListener.UrlOpeningListener(false)
+
+ // Last project and module to be able to use them when activated for configuration tasks.
+ var project: Project? = null


Should they really be nullable?

configureMockFramework(project!!, module!!) is very bad later

Yes, they should really be nullable. The other approach is to use lateinit var, but this will cause tight coupling with TestReportNotifier and force initialization of such properties at some point in the caller code, which is not obvious.

EgorkaKulikov · 2022-06-24T08:12:16Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/Notifications.kt

+
+ private fun handleDescription(descriptionSuffix: String) {
+ when {
+ descriptionSuffix.startsWith(mockitoSuffix) -> {


May be we should store a list of all possible suffixes and do smth like when here?

EgorkaKulikov · 2022-06-24T08:22:04Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/UtTestsDialogProcessor.kt

 }

+ forceMockListener?.run {
+ model.forceMockingHappened = forceMockHappened


Make the names similar, please

EgorkaKulikov · 2022-06-24T08:39:12Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/generator/TestGenerator.kt

 val notifyMessage = buildString {
 appendHtmlLine(testsCodeWithTestReport.testsGenerationReport.toString())
 appendHtmlLine()
+ if (model.forceMockingHappened) {


Actually it is not very scalable solution, but I'm not sure it is easy to do better.

CaelmBleidd · 2022-06-24T14:49:14Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/Notifications.kt

+ }
+ else {


Odd formatting

CaelmBleidd · 2022-06-24T14:50:23Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/Notifications.kt

+ descriptionSuffix.startsWith(mockitoSuffix) -> {
+ project?.let { module?.let {
+ if (createMockFrameworkNotificationDialog("Configure mock framework") == Messages.YES) {
+ configureMockFramework(project!!, module!!)


You can use it for the module and introduce a variable for the project. Otherwise, why would you use let instead of just if

CaelmBleidd · 2022-06-24T14:52:50Z

utbot-framework/src/main/kotlin/org/utbot/engine/Mocks.kt

+ mockInfo: UtMockInfo,
+ ): Boolean = doShouldMock(type, mockInfo).also { mockListenerController.onShouldMock(strategy, it) }
+
+ private fun doShouldMock(


I think it is a strange name. checkIfShouldMock or something like that might be more suitable

CaelmBleidd · 2022-06-24T14:57:50Z

utbot-framework/src/main/kotlin/org/utbot/engine/Mocks.kt

 private val strategy: MockStrategy,
 private val classUnderTest: ClassId,
 private val hierarchy: Hierarchy,
+ internal val mockListenerController: MockListenerController,


I'm not sure that it is a good idea for the Mocker to have a mandatory field with the controller. What if I don't know anything about it and just want to check whether some field would be mocked or not? What should I pass here? Or now Mocker cannot work without the engine (more specifically, without a current version of the engine, with the controller)

CaelmBleidd · 2022-06-24T14:58:53Z

utbot-intellij/src/main/kotlin/org/utbot/intellij/plugin/ui/UtTestsDialogProcessor.kt

+ ForceMockListener().apply {
+ codeGenerator.generator.configureEngine = { engine -> engine.attachMockListener(this) }
+ }
+ } else null


As I've mentioned many times, brackets

EgorkaKulikov · 2022-06-27T15:32:01Z

Approve in accordance with our verbal discussion on improvements

CaelmBleidd

There is an odd-looking connection between the mocker, its controllers, engine controllers and listeners. Seems like it is a problem with the design and it might cause further problems with engine refactorings and separation of the virtual machine and traverse

Approved since the PR contains required fix and need to be tested ASAP

Added mock listeners for engine. Added force mock listener. Added url handler for urls like "#utbot/...". Added message with link to mock framework configuration to test reports if force mocking happened.

nikitavlaev requested review from EgorkaKulikov and Vassiliy-Kudryashov June 22, 2022 09:51

nikitavlaev self-assigned this Jun 22, 2022

EgorkaKulikov reviewed Jun 24, 2022

View reviewed changes

CaelmBleidd requested changes Jun 24, 2022

View reviewed changes

EgorkaKulikov approved these changes Jun 27, 2022

View reviewed changes

CaelmBleidd approved these changes Jun 27, 2022

View reviewed changes

nikitavlaev force-pushed the nikitavlaev/force-mock-fix branch from 5bb59b3 to 1be046f Compare June 28, 2022 05:54

Force mocking without mock framework installed fix

3d34399

Added mock listeners for engine. Added force mock listener. Added url handler for urls like "#utbot/...". Added message with link to mock framework configuration to test reports if force mocking happened.

nikitavlaev force-pushed the nikitavlaev/force-mock-fix branch from 1be046f to 3d34399 Compare June 28, 2022 05:59

nikitavlaev enabled auto-merge (rebase) June 28, 2022 06:09

nikitavlaev merged commit 1ac3a04 into main Jun 28, 2022

nikitavlaev deleted the nikitavlaev/force-mock-fix branch June 28, 2022 07:21

nikitavlaev restored the nikitavlaev/force-mock-fix branch June 28, 2022 07:36

denis-fokin added the priority-top-focus Top priority chosen by dev team label Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Force mocking without mock framework installed fix #278

Force mocking without mock framework installed fix #278

Uh oh!

nikitavlaev commented Jun 22, 2022 •

edited

Loading

EgorkaKulikov Jun 23, 2022

EgorkaKulikov Jun 23, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

nikitavlaev Jun 27, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

EgorkaKulikov Jun 24, 2022

CaelmBleidd Jun 24, 2022

CaelmBleidd Jun 24, 2022

CaelmBleidd Jun 24, 2022

CaelmBleidd Jun 24, 2022

CaelmBleidd Jun 24, 2022

EgorkaKulikov commented Jun 27, 2022

CaelmBleidd left a comment

Labels

4 participants

Force mocking without mock framework installed fix #278

Force mocking without mock framework installed fix #278

Uh oh!

Conversation

nikitavlaev commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

How Has This Been Tested?

Manual Scenario

Checklist (remove irrelevant options):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EgorkaKulikov commented Jun 27, 2022

CaelmBleidd left a comment

Choose a reason for hiding this comment

Labels

4 participants

nikitavlaev commented Jun 22, 2022 •

edited

Loading