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

Disable daemon in noDaemonTest #6149

Conversation

blindpirate
Copy link
Collaborator

Context

In #6106 we disabled daemon for some certain
subprojects, however, all subprojects' noDaemonTest requires allDistribution, which
in turn invokes userguide task. This PR disabled daemon for noDaemonTest, given that
noDaemonTest is excuted once a day.

In #6106 we disabled daemon for some certain
subprojects, however, all subprojects' `noDaemonTest` requires allDistribution, which
in turn invokes userguide task. This PR disabled daemon for `noDaemonTest`, given that
noDaemonTest is excuted once a day.
@blindpirate blindpirate added from:member a:chore Minor issue without significant impact @dev-productivity Issue owned by developer-productivity labels Aug 1, 2018
@blindpirate blindpirate self-assigned this Aug 1, 2018
@blindpirate blindpirate requested a review from wolfs August 1, 2018 02:09
@@ -30,7 +30,7 @@ class FunctionalTest(model: CIBuildModel, testCoverage: TestCoverage, subProject
+ buildScanValues.map { buildScanCustomValue(it.key, it.value) }
).joinToString(separator = " "),
timeout = testCoverage.testType.timeout,
daemon = useDaemon)
daemon = useDaemon && testCoverage.testType != TestType.noDaemon)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should do that decision in the model. I guess this time FunctionTestProject would be the way to go.
Something like
val useDaemon = subProject.useDaemon && testConfig.testType != TestType.noDaemon.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only problem is, subProject can be null here, because

buildType(FunctionalTest(model, testCoverage, stage = stage))
this line won't pass subProject parameter at all, which makes the null-handling pretty annoying. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

subProject can't be null in FunctionalTestProject:

model.subProjects.forEach { subProject ->
if (shouldBeSkipped(subProject, testConfig)) {
return@forEach
}
if (subProject.containsSlowTests && stage.omitsSlowProjects) {
addMissingTestCoverage(testConfig)
return@forEach
}
if (subProject.unitTests && testConfig.testType.unitTests) {
buildType(FunctionalTest(model, testConfig, subProject.name, subProject.useDaemon, stage))
} else if (subProject.functionalTests && testConfig.testType.functionalTests) {
buildType(FunctionalTest(model, testConfig, subProject.name, subProject.useDaemon, stage))
} else if (subProject.crossVersionTests && testConfig.testType.crossVersionTests) {
buildType(FunctionalTest(model, testConfig, subProject.name, subProject.useDaemon, stage))
}
}

We only create the buildType(...) for soak tests - so useDaemon would be true there anyway, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, I also did a refactoring work to avoid some repetition.

import model.CIBuildModel
import model.Stage
import model.TestCoverage
import model.*
Copy link
Member

Choose a reason for hiding this comment

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

Let's not do * imports.

fun Project.configureBuildType(model: CIBuildModel, testConfig: TestCoverage, stage: Stage, subProject: GradleSubproject) {
val useDaemon = subProject.useDaemon && testConfig.testType != TestType.noDaemon

if (subProject.unitTests && testConfig.testType.unitTests) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess the whole if/else thing can be simplified by adding two methods to GradleSubproject:

fun hasTestsFor(type: TestType) = (unitTests && type.unitTests)
            || (functionalTests && type.functionalTests)
            || (crossVersionTests && type.crossVersionTests)

fun useDaemon(type: TestType) = useDaemon && (type != TestType.noDaemon)

Then the code simplifies to:

                        if (subProject.hasTestsOf(testConfig.testType)) {
                            buildType(FunctionalTest(model, testConfig, subProject.name, subProject.useDaemon(testConfig.testType), stage))
                        }

The we don't need this method any more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, brilliant idea.

@blindpirate blindpirate force-pushed the blindpirate/disable-daemon-in-nodaemon-test branch from 4a655c8 to 28bf652 Compare August 2, 2018 00:59
@blindpirate blindpirate merged commit 63dbd12 into teamcity-versioned-settings Aug 2, 2018
@big-guy big-guy deleted the blindpirate/disable-daemon-in-nodaemon-test branch May 22, 2019 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:chore Minor issue without significant impact @dev-productivity Issue owned by developer-productivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants