feat(query-generation): modal for specifying default values INTELLIJ-198 #133
Conversation
kmruiz
changed the title
Coverage Report
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...ects/mongosh/src/main/kotlin/com/mongodb/jbplugin/dialects/mongosh/backend/MongoshBackend.kt
Fixed
Show fixed
Hide fixed
|
🤖 Benchmark Comparison for
|
|
🤖 Benchmark Comparison for
|
There was a problem hiding this comment.
Pretty amazing work @kmruiz! I left a few small suggestions but overall it looks really great.
...ins-plugin/src/main/kotlin/com/mongodb/jbplugin/codeActions/impl/RunQueryCodeActionBridge.kt
Show resolved
Hide resolved
...s-plugin/src/main/kotlin/com/mongodb/jbplugin/codeActions/impl/runQuery/NamespaceSelector.kt
Show resolved
Hide resolved
...rains-plugin/src/main/kotlin/com/mongodb/jbplugin/codeActions/impl/runQuery/RunQueryModal.kt
Show resolved
Hide resolved
| class RunQueryModal( | ||
| private val query: Node<PsiElement>, | ||
| private val dataSource: LocalDataSource, | ||
| private val coroutineScope: CoroutineScope | ||
| ) : DialogWrapper(query.source.project, false) { |
There was a problem hiding this comment.
Should we make this UI component consistent with the way NamespaceSelector and the others are built? Keeping the actual swing component (DialogWrapper in this case) internal to the class and exposing only what is needed on the interface. I noticed that it helps a lot in keeping the coupling loose when composing components together.
There was a problem hiding this comment.
That's a good point. I don't have a strong opinion, do you want to discuss this and see tradeoffs? I can book some time today and we can have some quality conversation on this 😄 .
There was a problem hiding this comment.
Yea that sounds good. Lets do that.
| databaseComboBox.requireEnabled() | ||
| .requireItemCount(1) | ||
| .requireSelection("db1") |
There was a problem hiding this comment.
The API looks pretty amazing, great find <3
| @Nested | ||
| @DisplayName("when execute is triggered") |
There was a problem hiding this comment.
Is it because the new annotation does not support nested test or in general we don't want to use nested tests?
There was a problem hiding this comment.
I'm not sure if we want to use nested test after my findings. I've found that the nested tests don't inherit the whole context of the parent class, and parts of the IntegrationTestExtension were not working properly. I guess if we don't need context and the tests can run in parallel we can use nested without issues, but with extensions, it seems to be pretty flaky.
There was a problem hiding this comment.
Got it. I did come across some issues using nested tests alongside CodeInsight tests but did not dig deeper back then. Seems like the problem is the same with them not being able to share the parent context.
There was a problem hiding this comment.
Yeah, maybe we can specify some conventions on when to use nested tests. I find them useful, so maybe we can find them a place where they are safe to use? Or should we just drop them?
.../jetbrains-plugin/src/test/kotlin/com/mongodb/jbplugin/fixtures/IntegrationTestExtensions.kt
Outdated
Show resolved
Hide resolved
|
🤖 Benchmark Comparison for
|
Description
This PR is bigger than usual because it required a pretty big refactor on our test code due to incompatibilities between how tests in IntelliJ and AssertJ Swing handle headless mode.
Checklist
Open Questions