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

Fix Issue 390: Better generation process monitor #393

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ interface CustomProgressIndicator {
fun isCanceled(): Boolean
fun start()
fun stop()
fun isRunning(): Boolean
Copy link
Collaborator

@Vladislav0Art Vladislav0Art Oct 15, 2024

Choose a reason for hiding this comment

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

Note: It'll require publishing a new core version since the interface changes.
CC: @pderakhshanfar

}
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@ import com.intellij.openapi.project.Project
import org.jetbrains.research.testspark.bundles.plugin.PluginMessagesBundle
import org.jetbrains.research.testspark.core.monitor.DefaultErrorMonitor
import org.jetbrains.research.testspark.core.monitor.ErrorMonitor
import org.jetbrains.research.testspark.core.progress.CustomProgressIndicator

/**
* Manager used for monitoring the unit test generation process.
* It also limits TestSpark to generate tests only once at a time.
*/
class TestGenerationController {
private var isRunning: Boolean = false
var indicator: CustomProgressIndicator? = null

// errorMonitor is passed in many places in the project
// and reflects if any bug happened in the test generation process
Expand All @@ -26,6 +27,7 @@ class TestGenerationController {
private fun showGenerationRunningNotification(project: Project) {
val terminateButton: AnAction = object : AnAction("Terminate") {
override fun actionPerformed(e: AnActionEvent) {
indicator?.stop()
Copy link
Collaborator

Choose a reason for hiding this comment

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

[Only my thoughts below; no request to any action]:

I genuinely do not like that we bring a dependency on CustomProgressIndicator further in the control flow.

Valich suggested us to transition to coroutines and reduce our CustomProgressIndicator interface to a viewer of the indicator's text (i.e. it should merely allow setting the indicator's display text).

Note: Calling stop method on the IntelliJ's ProgressIndicator is usually discouraged, as I saw in its Javadoc.

I suggest a major refactoring somewhere in the future after the release to tackle this long-lasting issue with CustomProgressIndicator. There was an issue somewhere in this regard.

CC: @pderakhshanfar @Hello-zoka @arksap2002.

Comment on lines 28 to +30
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to close this popup modal after a user clicks "Terminate." Otherwise, I can click as many times as I like (it does not cause errors, though).

errorMonitor.notifyErrorOccurrence()
}
}
Expand All @@ -44,7 +46,11 @@ class TestGenerationController {
}

fun finished() {
isRunning = false
if (indicator != null &&
indicator!!.isRunning()
) {
Comment on lines +49 to +51
Copy link
Collaborator

Choose a reason for hiding this comment

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

the expression is quite short, so making it as this is fine:

if ((indicator != null) && indicator!!.isRunning()) {

indicator?.stop()
}
}

/**
Expand All @@ -53,11 +59,15 @@ class TestGenerationController {
* @return true if it is already running
*/
fun isGeneratorRunning(project: Project): Boolean {
if (isRunning) {
// If indicator is null, we have never initiated an indicator before and there is no running test generation
if (indicator == null) {
return false
}

if (indicator!!.isRunning()) {
Comment on lines +62 to +67
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about:

fun isGeneratorRunning(project: Project): Boolean {
  // If indicator is null, we have never initiated an indicator before and there is no running test generation
  return indicator?.let { it.isRunning() } ?: false;
}

// OR:

fun isGeneratorRunning(project: Project): Boolean = indicator?. let { it.isRunning() } ?: false;

showGenerationRunningNotification(project)
return true
}
isRunning = true
return false
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.intellij.openapi.editor.Editor
import com.intellij.openapi.project.Project
import com.intellij.openapi.wm.ToolWindow
import com.intellij.openapi.wm.ToolWindowManager
import com.intellij.serviceContainer.AlreadyDisposedException
import com.intellij.ui.content.ContentManager
import org.jetbrains.research.testspark.bundles.plugin.PluginLabelsBundle
import org.jetbrains.research.testspark.bundles.plugin.PluginMessagesBundle
Expand Down Expand Up @@ -94,7 +95,8 @@ class TestSparkDisplayManager {
}
}
}

toolWindow?.hide()
try {
toolWindow?.hide()
} catch (_: AlreadyDisposedException) {} // Make sure the process continues if the tool window is already closed
Comment on lines +98 to +100
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's better to log the error at least; silencing it potentially may bring problems in the future.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class IJProgressIndicator(private val indicator: ProgressIndicator) : CustomProg

override fun isCanceled(): Boolean = indicator.isCanceled

override fun isRunning(): Boolean = indicator.isRunning

override fun start() {
indicator.start()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package org.jetbrains.research.testspark.display.generatedTests
import com.intellij.openapi.editor.Editor
import com.intellij.openapi.project.Project
import com.intellij.openapi.wm.ToolWindowManager
import com.intellij.serviceContainer.AlreadyDisposedException
import com.intellij.ui.content.ContentFactory
import com.intellij.ui.content.ContentManager
import org.jetbrains.research.testspark.bundles.plugin.PluginLabelsBundle
Expand Down Expand Up @@ -233,9 +234,11 @@ class GeneratedTestsTabBuilder(
* Closes the tool window by removing the content and hiding the window.
*/
private fun closeToolWindow() {
generatedTestsTabData.contentManager?.removeContent(generatedTestsTabData.content!!, true)
ToolWindowManager.getInstance(project).getToolWindow("TestSpark")?.hide()
coverageVisualisationTabBuilder.closeToolWindowTab()
try {
generatedTestsTabData.contentManager?.removeContent(generatedTestsTabData.content!!, true)
ToolWindowManager.getInstance(project).getToolWindow("TestSpark")?.hide()
coverageVisualisationTabBuilder.closeToolWindowTab()
} catch (_: AlreadyDisposedException) {} // Make sure the process continues if the tool window is already closed
Copy link
Collaborator

Choose a reason for hiding this comment

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

logging

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ class HeadlessProgressIndicator : CustomProgressIndicator {

override fun isCanceled(): Boolean = false

override fun isRunning(): Boolean = true

override fun start() {}

override fun stop() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ class Pipeline(
override fun run(indicator: ProgressIndicator) {
try {
val ijIndicator = IJProgressIndicator(indicator)
testGenerationController.indicator = ijIndicator

if (ToolUtils.isProcessStopped(testGenerationController.errorMonitor, ijIndicator)) return

Expand Down
Loading