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

IDEA sync smoke tests #27838

Closed
wants to merge 1 commit into from
Closed

Conversation

6hundreds
Copy link
Member

This PR is introducing project import to IDEA tests.

  1. The logic of IDE provisioning(downloading and configuring) is done on gradle-profiler site :
    Add IDEA provisioning gradle-profiler#529
    Studio provisioning gradle-profiler#530
    Essentially this is eliminating android-studio-provisioning plugin usage from smoke-ide-test subproject.

Profiler provisioning support is in a very early alpha, since there are some decisions need to take. The main issue is that usage of ide-starter in profiler is requiring Java 17 while profiler is a Java 8 application. This PR is using special Java 17 version of profiler, but this is not a final decision.

Using ide-starter is bringing many of transitive dependencies to gradle/gradle what is making me unhappy. I believe we should shade ide-starter in profiler, since it's never intended(and will not I hope) to be an public API. This also leading to bloaing verification-metadata.xml.

  1. This PR is also making smokeIdeTests are regular integTests.
  2. This PR starts to using IntegrationTestBuildContext for providing shared gradleUserHome and a home for IDEs.

@6hundreds 6hundreds requested review from a team as code owners January 26, 2024 15:24
@6hundreds 6hundreds requested review from abstratt, alllex and cobexer and removed request for a team January 26, 2024 15:24
@6hundreds 6hundreds self-assigned this Jan 26, 2024
@6hundreds 6hundreds requested review from bamboo and blindpirate and removed request for cobexer and abstratt January 26, 2024 15:25

sync(
"IC",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this might be implementation details, but "IC" sounds like "IntelliJ Community". Is it possible to make it a constant, like:

private static final INTELLIJ_COMMUNITY_TYPE = "IC";

sync(INTELLIJ_COMMUNITY_TYPE, ...)

Same to the AI above.

@@ -850,7 +850,7 @@
"name": "smoke-ide-test",
"path": "subprojects/smoke-ide-test",
"unitTests": false,
"functionalTests": false,
"functionalTests": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will cause :smoke-ide-test subproject to be run in ALL variants, i.e.

  • QuickFeedbackLinux: :smoke-ide-test:quickTest
  • QuickFeedback: :smoke-ide-test:quickTest
  • Platform Linux: :smoke-ide-test:platformTest
  • ...

This is not intended, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@blindpirate Right, this is not intended. We want it to run like a smoke tests.
Also, this PR is changing source set from smokeIdeTest to a regular integTest. Will it lead to unintentional run smoke-ide-tests during other check, that involving running of all integTests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it lead to unintentional run smoke-ide-tests during other check, that involving running of all integTests?

Yes. I think this should be visible if you run a build on your branch.

Copy link
Member Author

Choose a reason for hiding this comment

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

@blindpirate We want to avoid this. Is there other way apart from using another sourceSet? But if it's a most recommended way, we could revert that sourceSet changes back

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you try "functionalTests": false unchanged? I think this is how we recognize regular integTest.

Copy link
Member Author

Choose a reason for hiding this comment

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

@blindpirate Yes, I can try it, but at the same time it will brings some implicit behavior:
When you see integTest source set, it's implies that it will run with other functional tests. And if I'll make "functionalTests": false it can be confusing a bit.

From other side, having a dedicated source set (as it was before) is explicitly tells that there is a logic to run it accordingly.

WDYT? cc @bamboo

Copy link
Member Author

Choose a reason for hiding this comment

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

@blindpirate Huh, when I'm making "functionalTests": false (unchanged essentially), then :checkSubprojectsInfo is failing:

:checkSubprojectsInfo'. org.gradle.api.GradleException: New project(s) added without updating subproject JSON. Please run `:generateSubprojectsInfo`

:generateSubprojectsInfo, in it's turn, is generating "functionalTests": true for smoke-ide-test, what is breaking this condition.

So apparently, it would be correct to revert source set changes

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. So the problem here is the violation of our common convention:

  • If src/integTest exists, we'll consider it as "functionalTests": true.
  • Then we'll test it in different OS/executorType/JavaVersion combinations.

So there're a few options in my mind:

  1. Stop using integTest as the location for the new IDE tests, do as smoke-test subproject does (the old way).
  2. Write a special if block in checkSubprojectsInfo and generateSubprojectsInfo to handle this "exception".
  3. Add @Requires() annotation to your smoke ide test class, so that the test class will be skipped in the OS/executorType/JavaVersion it's not intended to be run.

Personally I like option 3).

@6hundreds 6hundreds requested a review from a team as a code owner February 3, 2024 14:23
@gradle gradle deleted a comment from 6hundreds Feb 9, 2024
@gradle gradle deleted a comment from 6hundreds Mar 4, 2024
@gradle gradle deleted a comment from 6hundreds Mar 7, 2024
@gradle gradle deleted a comment from 6hundreds Mar 7, 2024
@gradle gradle deleted a comment from 6hundreds Mar 13, 2024
@gradle gradle deleted a comment from 6hundreds Mar 13, 2024
@gradle gradle deleted a comment from 6hundreds Mar 15, 2024
@gradle gradle deleted a comment from 6hundreds Mar 15, 2024
@6hundreds
Copy link
Member Author

@bot-gradle test prf

@gradle gradle deleted a comment from 6hundreds Mar 15, 2024
@bot-gradle
Copy link
Collaborator

I've triggered the following builds for you. Click here to see all build failures.

@6hundreds
Copy link
Member Author

@bot-gradle test prf

@gradle gradle deleted a comment from 6hundreds Mar 18, 2024
@bot-gradle
Copy link
Collaborator

I've triggered the following builds for you. Click here to see all build failures.

@6hundreds 6hundreds force-pushed the 6hundreds/27553/idea-provisioning branch from f005e1b to 1231278 Compare March 18, 2024 09:45
@bamboo
Copy link
Member

bamboo commented Apr 8, 2024

Is this still planned?

@6hundreds
Copy link
Member Author

Superseded with #28526

@6hundreds 6hundreds closed this Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants