-
Notifications
You must be signed in to change notification settings - Fork 330
Bootstrap the Flutter IJ with integrations tests #8487
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
Conversation
e5d908a
to
720e408
Compare
720e408
to
2494c6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
testSrc/integration/io/flutter/integrationTest/NewProjectUITest.kt
Outdated
Show resolved
Hide resolved
class MyProjectUITest { | ||
|
||
companion object { | ||
// Generate a unique folder name for the test project to avoid conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an odd place for the comment. It feels like it should be in initContext
. (Unless you want to say: "this will be updated to a unique folder name..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks!
testSrc/integration/io/flutter/integrationTest/utils/NewProject.kt
Outdated
Show resolved
Hide resolved
The additional STARTER test framework is added, see https://github.com/JetBrains/intellij-ide-starter Thanks to others here who trailblazed, @jonathan1983, JetBrains/intellij-platform-plugin-template#537 and @helinx, flutter#8338 The change does not try to get the new tests working in the presubmit.
1d21155
to
d041a13
Compare
* found in the LICENSE file. | ||
*/ | ||
|
||
package io.flutter.integrationTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed this before. but the standard practice is to keep package names all lowercase. See:
https://docs.oracle.com/javase/tutorial/java/package/namingpkgs.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you address this in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolving in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting!
return setupTestContext( | ||
"", IdeProductProvider.IC.copy( | ||
// TODO(team) should the version be fetched from some setting, i.e. System.getProperty("uiPlatformBuildVersion") | ||
buildNumber = "252.23892.409", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we inject from build.gradle.kts
and/or from the gradle.properties
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can. In the task setup in the gradle configuration file you pass the gradle property along as a JVM property. resolving these TODOs as well as threading through the dart plugin version as a follow up.
|
||
@Test | ||
@Disabled("Need license configuration to test") | ||
fun newProjectWS() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to call this something like testNewProjectWS
? Maybe this is unnecessary with the annotation, but I find it hard to know where the test originates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up with another PR
The additional STARTER test framework is added, see https://github.com/JetBrains/intellij-ide-starter
Thanks to others here who trailblazed, @jonathan1983, JetBrains/intellij-platform-plugin-template#537 and @helinx, #8338