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

Integration test project #3307

Merged
merged 18 commits into from
Jun 15, 2022
Merged

Integration test project #3307

merged 18 commits into from
Jun 15, 2022

Conversation

mvicsokolova
Copy link
Contributor

No description provided.

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented May 31, 2022

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

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

suspend fun doWorld() = coroutineScope {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

integration-test/settings.gradle Outdated Show resolved Hide resolved
integration-test/gradlew Outdated Show resolved Hide resolved
@qwwdfsad
Copy link
Collaborator

qwwdfsad commented Jun 6, 2022

@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
Copy link
Collaborator

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:

  1. 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.
  1. 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
Copy link
Collaborator

qwwdfsad commented Jun 7, 2022

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 qwwdfsad self-requested a review June 7, 2022 13:04
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

.

@dkhalanskyjb
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

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

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"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

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

integration-testing/build.gradle Outdated Show resolved Hide resolved
integration-testing/smokeTest/build.gradle Outdated Show resolved Hide resolved
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"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

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

@dkhalanskyjb dkhalanskyjb self-requested a review June 14, 2022 14:02
mvicsokolova and others added 2 commits June 14, 2022 22:41
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

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

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

@mvicsokolova
Copy link
Contributor Author

Updated the README 🙂

@qwwdfsad qwwdfsad merged commit f101f93 into develop Jun 15, 2022
@qwwdfsad qwwdfsad deleted the integration-test-branch branch June 15, 2022 17:22
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
Test for Kotlin#3305 

Co-authored-by: Vsevolod Tolstopyatov <qwwdfsad@gmail.com>
Co-authored-by: dkhalanskyjb <52952525+dkhalanskyjb@users.noreply.github.com>
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.

None yet

3 participants