Add test for CMP-7505 #2312
Add test for CMP-7505 #2312
Conversation
.idea/vcs.xml
Outdated
There was a problem hiding this comment.
thanks, havent noticed it! :)
...aterial3/material3/src/skikoTest/kotlin/androidx/compose/material3/WideNavigationRailTest.kt
Outdated
Show resolved
Hide resolved
| @OptIn(ExperimentalTestApi::class) | ||
| class WideNavigationRailTest { | ||
| @Test | ||
| fun `check CMP-7505 compiles`() = runComposeUiTest { |
There was a problem hiding this comment.
Could you check if reverting your fix fails the compilation of this Test?
There was a problem hiding this comment.
ohhh, test were in same module as classes-to-be-tested. it caused it not to fail, because compilation failure only happens on compilation in different modules
so ive moved the sample to mpp/demo and it works with compose plugin adjustment and it fails without it
There was a problem hiding this comment.
We don't compile demo in any CI check right now, so we need to add its compilation somewhere.
But I would not change the demo, as its purpose is not for auto testing. We can move it, for example, to a new module mpp/compilation-tests. And then register its compilation here in all test tasks.
There was a problem hiding this comment.
@igordmn are you sure that it's good idea to add it mpp/compilation-tests with pinned versions? I think something like https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates is much better fit and might be triggered by each dev
There was a problem hiding this comment.
are you sure that it's good idea to add it mpp/compilation-tests with pinned versions
It should be not pinned, indeed.
I think something like https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates is much better fit and might be triggered by each dev
The test is about compilation with Compose Compiler, so it looks okay to keep it here instead of https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates, which purpose is for end-to-end checks.
There was a problem hiding this comment.
It's kinda e2e thing. Also this change basically converts mpp from "just app for debugging" to infra meaningful thing. Which is questionable if we consider mpp folder as a thing that will be migrated to google test app (integration-tests) eventually.
There was a problem hiding this comment.
If you think that it is better to have it https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates, I have no objections for that.
Will you request this change?
There was a problem hiding this comment.
Already requested. Added more detailed comment in that thread
There was a problem hiding this comment.
Let's move the conversation there
shishkin-pavel
force-pushed
the
shish/CMP-7505/add-test
branch
from
b299705 to
56a778a
Compare
| exclude("org.jetbrains.compose.ui") | ||
| } | ||
|
|
||
| implementation("org.jetbrains.compose.material3:material3:1.9.0-beta03") |
There was a problem hiding this comment.
With this approach it won't really test any future changes
There was a problem hiding this comment.
Currently, with unpinned dependencies it might work, but it basically introduces new e2e test concept in this repo. It's might be good in long term, but I beleive that it should be done via commonization of Google's integration-tests instead adding more stuff in mpp folder. I guess it's out of the scope here.
I mean I see this mpp folder as "just app for debugging" and eventually needs to be removed, but by introducing infra related stuff to it, you changes the status of it and require more work during migration to integration-tests.
My suggestion is to add such tests in https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates where our e2e placed now
There was a problem hiding this comment.
After adding it to this folder, we also need to add it here https://jetbrains.team/p/ui/repositories/compose-teamcity-config/files/main/.teamcity/compose/publish/subtasks/Task4ValidateTemplatesTutorials.kt
shishkin-pavel
force-pushed
the
shish/CMP-7505/add-test
branch
from
7be5fad to
439d73f
Compare
| id("AndroidXPlugin") | ||
| id("AndroidXComposePlugin") | ||
| id("kotlin-multiplatform") | ||
| // [1.4 Update] id("application") |
There was a problem hiding this comment.
| // [1.4 Update] id("application") |
| @OptIn(ExperimentalTestApi::class) | ||
| class WideNavigationRailTest { | ||
| @Test | ||
| fun `check CMP-7505 compiles`() = runComposeUiTest { |
There was a problem hiding this comment.
are you sure that it's good idea to add it mpp/compilation-tests with pinned versions
It should be not pinned, indeed.
I think something like https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates is much better fit and might be triggered by each dev
The test is about compilation with Compose Compiler, so it looks okay to keep it here instead of https://github.com/JetBrains/compose-multiplatform/tree/master/ci/templates, which purpose is for end-to-end checks.
| } | ||
|
|
||
| kotlin { | ||
| jvm() |
There was a problem hiding this comment.
Don't we need to include iOS too? Or are there issues including it?
verifies CMP-7505
Release Notes
N/A