Integration test project #3307

mvicsokolova · 2022-05-31T13:28:24Z

No description provided.

dkhalanskyjb · 2022-05-31T14:09:21Z

Don't know the specifics here, so no idea if I'm saying something useful, and if I'm not, just disregard this.

Maybe a cleaner approach would be to create a source-set in the integration-testing subproject? I think that that subproject is isolated from atomicfu, as mavenTest, for example, checks already that atomicfu is not there: https://github.com/Kotlin/kotlinx.coroutines/blob/master/integration-testing/src/mavenTest/kotlin/MavenPublicationAtomicfuValidator.kt#L15-L18

qwwdfsad · 2022-06-06T09:17:01Z

integration-test/src/commonMain/kotlin/main.kt

@@ -0,0 +1,9 @@
+import kotlinx.coroutines.*
+
+suspend fun doWorld() = coroutineScope {


Please add it as @Test, so this code will be ran as well

integration-test/settings.gradle

integration-test/gradlew

integration-test/gradle.properties

qwwdfsad · 2022-06-06T09:21:47Z

@dkhalanskyjb it is possible to do so, but such an approach may be not robust enough -- it's easy to have some dependency slipped into subproject via all our configure.all blocks and defeat the purpose of the test: whether a standalone, fresh MPP project can depend on kotlinx.coroutines (see #3305)

Taking it into account, could you please also take a look at this PR?

dkhalanskyjb · 2022-06-07T11:15:44Z

Pros of having a separate subproject:

If we make a mistake in the build scripts, globally applying some plugin for all subprojects and not excluding the integration testing one, we may miss a production issue that will be caught on the RC stage (if it's not caught then, the smoke test will not do much good). The dependencies can be handled fairly robustly: Make the mavenTest integration test more robust #3315

Cons:

The subproject is not recognized for automatic refactorings (this was the problem that initially led us to move mavenTest to be in a subproject and not just lying nearby).
A complication of the build process.
As implemented, a complication of the project structure, as we already have integration-testing, and the split between their responsibilities is very vague. Can be mitigated, see below.

Another consideration is that this problem is not limited to the coroutines library. For example, we encountered a similar issue with the datetime, where the project itself behaved just fine, but projects using it, but not the serialization plugin, did not.

Maybe this is a sign that we need a more general solution for this issue, like the ability to have subprojects that run the Kotlin compiler only with the stock arguments, which would allow us to have all the pros without any of the cons. Do we maybe have such an ability already? If so, we should employ that.

If not, let's at least reduce the complexity of the file structure. I see these solutions:

Clarify the separation between various types of integration tests.

Instead of just making a project called integration-test, introduce a more general publication-testing, for testing the published versions. This includes smokeTest (I think this is a more fitting name for what is now integration-test), npmTest, and mavenTest.
Keep integration-testing for tests that depend on the other subprojects—for now, those are debugAgentTest and coreAgentTest.

Change integration-testing not to be a subproject, so that all integration tests run on published versions, and let integration-test be one of its tests.

qwwdfsad · 2022-06-07T13:04:00Z

Maybe this is a sign that we need a more general solution for this issue, like the ability to have subprojects that run the Kotlin compiler only with the stock arguments, which would allow us to have all the pros without any of the cons. Do we maybe have such an ability already? If so, we should employ that.

I am not sure about stock arguments and "some default behaviour". At the end of the day, every project, whether its coroutines, datetime or serialization itself, has its own constraints. What we are basically doing here is a tailored integration test, incorporated into the same repository for the sake of simplicity.

And especially I don't think it's worth relying on Gradle which is on its own complex enough and have enough moving parts to have a loophole to let any non-trivial mistake slip in. In my experience, almost every plugin or non-trivial configuration in our Gradle breaks, and breaks more often than it is reasonably expected to.

Change integration-testing not to be a subproject, so that all integration tests run on published versions, and let integration-test be one of its tests.

I would prefer to stick to this approach, @mvicsokolova could you please change the PR accordingly? Also, please note that there is no npmTest in develop

qwwdfsad

.

dkhalanskyjb · 2022-06-07T13:21:36Z

At the end of the day, every project, whether its coroutines, datetime or serialization itself, has its own constraints.

In general, sure, but they all need a way to be used from an out-of-the-box Kotlin project with only minimal configuration.

What we are basically doing here is a tailored integration test, incorporated into the same repository for the sake of simplicity.

Looking at the gradle configuration, I don't see anything tailored. On the contrary, it looks like it just came out from the project creation wizard.

And especially I don't think it's worth relying on Gradle which is on its own complex enough and have enough moving parts to have a loophole to let any non-trivial mistake slip in.

I don't have enough knowledge about how the Kotlin gradle plugins work, but I expect us to control most of that, probably except the dependency resolution. Likewise, all the compiler-argument-changing behavior is surely not implemented by calling kotlinc directly; instead, it must use the main Kotlin gradle plugin, where we could add an escape hatch of sorts that, given a whitelist of the -X flags and the compiler plugins, would only allow them, failing if something else was passed to it.

dkhalanskyjb · 2022-06-07T13:31:03Z

Also, my continuing the discussion is not a sign that I'm against us going forward with the current solution. We do need to publish something quickly, and the maintenance burden is not high, so a more clean solution can wait.

dkhalanskyjb

Please rebase on top of develop, so that git is not confused.

dkhalanskyjb · 2022-06-10T08:32:57Z

integration-testing/build.gradle

-    dependsOn(project(':kotlinx-coroutines-debug').shadowJar)
-    jvmArgs ('-javaagent:' + project(':kotlinx-coroutines-debug').shadowJar.outputs.files.getFiles()[0])
+    def coroutinesDebugJar = sourceSet.runtimeClasspath.filter {it.name == "kotlinx-coroutines-debug-${coroutines_version}.jar" }.singleFile
+    jvmArgs ('-javaagent:' + coroutinesDebugJar)
    testClassesDirs = sourceSet.output.classesDirs
    classpath = sourceSet.runtimeClasspath
    systemProperties project.properties.subMap(["overwrite.probes"])


Now, that's an issue.

This flag control the functionality in integration-testing/src/debugAgentTest/kotlin/PrecompiledDebugProbesTest.kt that allows us to replace DebugProbesKt.bin, a file in the repository, with a new version. I'm sure this functionality is now broken. We should either fix this or adapt in some other way. @qwwdfsad, do we still need -Poverwrite.probes? Does anyone rely but us on this, or can we provide a more fitting interface? Also, is the comment below still relevant (from the file with the test)?

ensure that classfile has major version equal to 50 (Java 6 compliance)

The comment is indeed redundant.

We should either fix this or adapt in some other way

Right. @mvicsokolova please ensure that ./gradlew debugAgentTest -Poverwrite.probes=true properly works. For now it fails with FileNotFoundException

Apart from that, ./gradlew publishToMavenLocal + cd integration-testing + ./gradlew debugAgentTest fails locally at PrecompiledDebugProbesTest. It shouldn't be be the case, please investigate

qwwdfsad

@dkhalanskyjb could you please take a look if general Gradle project structure looks good to you?

integration-testing/build.gradle

integration-testing/smokeTest/build.gradle

qwwdfsad · 2022-06-14T11:16:30Z

integration-testing/build.gradle

-    dependsOn(project(':kotlinx-coroutines-debug').shadowJar)
-    jvmArgs ('-javaagent:' + project(':kotlinx-coroutines-debug').shadowJar.outputs.files.getFiles()[0])
+    def coroutinesDebugJar = sourceSet.runtimeClasspath.filter {it.name == "kotlinx-coroutines-debug-${coroutines_version}.jar" }.singleFile
+    jvmArgs ('-javaagent:' + coroutinesDebugJar)
    testClassesDirs = sourceSet.output.classesDirs
    classpath = sourceSet.runtimeClasspath
    systemProperties project.properties.subMap(["overwrite.probes"])


The comment is indeed redundant.

We should either fix this or adapt in some other way

Right. @mvicsokolova please ensure that ./gradlew debugAgentTest -Poverwrite.probes=true properly works. For now it fails with FileNotFoundException

qwwdfsad · 2022-06-14T11:17:25Z

integration-testing/build.gradle

-    dependsOn(project(':kotlinx-coroutines-debug').shadowJar)
-    jvmArgs ('-javaagent:' + project(':kotlinx-coroutines-debug').shadowJar.outputs.files.getFiles()[0])
+    def coroutinesDebugJar = sourceSet.runtimeClasspath.filter {it.name == "kotlinx-coroutines-debug-${coroutines_version}.jar" }.singleFile
+    jvmArgs ('-javaagent:' + coroutinesDebugJar)
    testClassesDirs = sourceSet.output.classesDirs
    classpath = sourceSet.runtimeClasspath
    systemProperties project.properties.subMap(["overwrite.probes"])


Apart from that, ./gradlew publishToMavenLocal + cd integration-testing + ./gradlew debugAgentTest fails locally at PrecompiledDebugProbesTest. It shouldn't be be the case, please investigate

dkhalanskyjb

Yeah, Maria and I discussed the structure already, I don't think we can do much better than that here.

integration-testing/src/debugAgentTest/kotlin/PrecompiledDebugProbesTest.kt

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

dkhalanskyjb

Also, integration-testing/README.md is now inaccurate.

mvicsokolova · 2022-06-15T11:30:52Z

Updated the README 🙂

Test for Kotlin#3305 Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com> Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

qwwdfsad suggested changes Jun 6, 2022

View reviewed changes

qwwdfsad reviewed Jun 6, 2022

View reviewed changes

integration-test/gradle.properties Outdated Show resolved Hide resolved

qwwdfsad self-requested a review June 7, 2022 13:04

qwwdfsad suggested changes Jun 7, 2022

View reviewed changes

dkhalanskyjb reviewed Jun 10, 2022

View reviewed changes

mvicsokolova and others added 12 commits June 10, 2022 13:18

integration-test

ba4a93d

~comment

f204b21

Regenerated gradle wrapper

0f9d689

Removed dev repo, mavenLocal() is enough

f68953a

A common test added

9d0ac0e

Update version in integration-test by bump-version.sh

8e09712

build.gradle fix

ec04fa9

Integration-testing removed from subprojects, npmTest deleted

3754a2c

gradle wrapper for integration-testing

fa1ac5f

Simplify mavenTest config

a51ab66

smokeTest added as a subproject to integration-testing

5b42bd7

Fixed getting a jar file from the classpath

d8bc9e4

mvicsokolova force-pushed the integration-test-branch branch from 3dcca74 to d8bc9e4 Compare June 10, 2022 10:22

qwwdfsad suggested changes Jun 14, 2022

View reviewed changes

qwwdfsad requested a review from dkhalanskyjb June 14, 2022 11:17

dkhalanskyjb reviewed Jun 14, 2022

View reviewed changes

mvicsokolova added 3 commits June 14, 2022 14:33

Minor fixes

66af8a0

Remove comment

2669b2c

debugAgentTest fix: find and overwrite DebugProbesKt.bin

ad5425f

dkhalanskyjb self-requested a review June 14, 2022 14:02

dkhalanskyjb requested changes Jun 14, 2022

View reviewed changes

integration-testing/src/debugAgentTest/kotlin/PrecompiledDebugProbesTest.kt Outdated Show resolved Hide resolved

integration-testing/src/debugAgentTest/kotlin/PrecompiledDebugProbesTest.kt Outdated Show resolved Hide resolved

mvicsokolova and others added 2 commits June 14, 2022 22:41

Fix for getting the path of DebugProbesKt.bin

67936ac

Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>

Read and rewrite DebugProbesKt.bin

d50c23d

dkhalanskyjb requested changes Jun 15, 2022

View reviewed changes

README.md updated

fa41006

dkhalanskyjb approved these changes Jun 15, 2022

View reviewed changes

qwwdfsad merged commit f101f93 into develop Jun 15, 2022

qwwdfsad deleted the integration-test-branch branch June 15, 2022 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test project #3307

Integration test project #3307

mvicsokolova commented May 31, 2022

dkhalanskyjb commented May 31, 2022 •

edited

Loading

qwwdfsad Jun 6, 2022

qwwdfsad commented Jun 6, 2022

dkhalanskyjb commented Jun 7, 2022

qwwdfsad commented Jun 7, 2022

qwwdfsad left a comment

dkhalanskyjb commented Jun 7, 2022

dkhalanskyjb commented Jun 7, 2022

dkhalanskyjb left a comment

dkhalanskyjb Jun 10, 2022

qwwdfsad Jun 14, 2022

qwwdfsad Jun 14, 2022

qwwdfsad left a comment

qwwdfsad Jun 14, 2022

qwwdfsad Jun 14, 2022

dkhalanskyjb left a comment •

edited

Loading

dkhalanskyjb left a comment

mvicsokolova commented Jun 15, 2022

		@@ -0,0 +1,9 @@
		import kotlinx.coroutines.*

		suspend fun doWorld() = coroutineScope {

Integration test project #3307

Integration test project #3307

Conversation

mvicsokolova commented May 31, 2022

dkhalanskyjb commented May 31, 2022 • edited Loading

qwwdfsad Jun 6, 2022

Choose a reason for hiding this comment

qwwdfsad commented Jun 6, 2022

dkhalanskyjb commented Jun 7, 2022

qwwdfsad commented Jun 7, 2022

qwwdfsad left a comment

Choose a reason for hiding this comment

dkhalanskyjb commented Jun 7, 2022

dkhalanskyjb commented Jun 7, 2022

dkhalanskyjb left a comment

Choose a reason for hiding this comment

dkhalanskyjb Jun 10, 2022

Choose a reason for hiding this comment

qwwdfsad Jun 14, 2022

Choose a reason for hiding this comment

qwwdfsad Jun 14, 2022

Choose a reason for hiding this comment

qwwdfsad left a comment

Choose a reason for hiding this comment

qwwdfsad Jun 14, 2022

Choose a reason for hiding this comment

qwwdfsad Jun 14, 2022

Choose a reason for hiding this comment

dkhalanskyjb left a comment • edited Loading

Choose a reason for hiding this comment

dkhalanskyjb left a comment

Choose a reason for hiding this comment

mvicsokolova commented Jun 15, 2022

dkhalanskyjb commented May 31, 2022 •

edited

Loading

dkhalanskyjb left a comment •

edited

Loading