-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add tests for lifecycle callbacks access to surrounding methods #29164
Conversation
println("project name = " + p.name) | ||
} | ||
|
||
allprojects { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ What value are we getting from this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we discussed, this test mirrors the similar one for Groovy DSL and locks in the behavior that, in Kotlin DSL, there is no violation.
Line 669 in 2d1a6c0
def "build script can query basic details of projects in a #description called from allprojects block"() { |
Perhaps the test needs a better name. Do you have ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still feels a bit wasteful in compute time. I mean, this would/should be covered elsewhere by tests that use allprojects
when running with isolatedProjectsIntegTest
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have thought the same about the Groovy member resolution that was not caught by tests before 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, we're just now adding Isolated Projects test coverage to :core
(#29202) and we do see some Groovy related errors there. No harm in having some explicit test coverage I guess.
} | ||
|
||
gradle.lifecycle.beforeProject { | ||
Helper.printInfo(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
gradle.lifecycle.beforeProject { | ||
${owner}.printInfo(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
"static" | "static " | "Helper" | ||
} | ||
|
||
@ToBeImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
// outputContains("project name = b") | ||
} | ||
|
||
@ToBeImplemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
println("project name = " + p.name) | ||
} | ||
|
||
allprojects { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still feels a bit wasteful in compute time. I mean, this would/should be covered elsewhere by tests that use allprojects
when running with isolatedProjectsIntegTest
...
Test-only PR that capture the current behavior of the
gradle.lifecycle
callbacks with respect to access of functions outside of the callback lambdas.