Upgrade AGP to 7.3.0, Gradle to 7.6 #1075
Conversation
| @@ -380,6 +381,7 @@ class GenerateResourcesIntegrationTest : IntegrationTestBase(), SharedIntegratio | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore("Previously this test worked because empty catalogs were generated and passed into the task") | |||
There was a problem hiding this comment.
To be verified if this test/implementation for it should be rewritten
There was a problem hiding this comment.
The check for this is here:
This test absolutely has to pass because we rely on it for correctness. The way we pick which resource to use for a particular flavor is dependent on the parent of the play directory being a valid flavor name:
Anyway, off the top of my head I think you can fix name != PLAY_PATH by combining two climbUpTos and if both of them are non-null then you have two play directories. There might be a cleaner way though.
There was a problem hiding this comment.
@SUPERCILEX could you please elaborate more on what this test is supposed to verify? 🙏
Looking at those two it seems like those test are identical except for different flavor name - it says "play file name is not allowed", but I can't grasp my head around it - it sounds like only "play" flavor is allowed and others are not (which is unexpected I guess)?
@Test
fun `Flavor named 'play' is allowed`() {
// language=gradle
val config = """
flavorDimensions 'pricing'
productFlavors {
play { dimension 'pricing' }
}
""".withAndroidBlock()
val result = execute(config, "generatePlayReleasePlayResources")
result.requireTask(taskName("PlayRelease"), outcome = SUCCESS)
}
@Test
@Ignore("Previously this test worked because empty catalogs were generated and passed into the task")
fun `'play' file name is not allowed`() {
// language=gradle
val config = """
flavorDimensions 'pricing'
productFlavors {
illegalPlay { dimension 'pricing' }
}
""".withAndroidBlock()
val result = executeExpectingFailure(
config, "generateIllegalPlayReleasePlayResources")
result.requireTask(taskName("IllegalPlayRelease"), outcome = FAILED)
}Please read my PR's description on how now parameters provided to
abstract class Validator : WorkAction<Validator.Params> {
override fun execute() {
for (file in parameters.files.get()) {
file.validate()
}
}have changed with the recent version.
I'm happy to improve it but would really appreciate your explanation in here 😄
There was a problem hiding this comment.
Shoot, forgot about that, makes life more complicated. For understanding the tests, it's easiest to look at the fixtures: https://github.com/Triple-T/gradle-play-publisher/tree/master/play/plugin/src/test/fixtures/GenerateResourcesIntegrationTest/src. Every directory in there is a flavor, so you'll find the play and illegalPlay dirs in there.
I've looked at this some more and I think we might be able to fix this by replacing it.asFile == this with it.asFile == climbUpTo(PLAY_PATH). That check is looking for file names that are play, but not part of the root input dirs. So if we look through every input file and find its parent play dir, it should always be a root play dir (I think).
There was a problem hiding this comment.
@SUPERCILEX please take a look at dbcd5a2.
My confusion came from this test's name being a little bit misleading I think.
From what I understand now is that those two cases are illegal:
-
One is not allowed to have any files named "play"
project/my-project/src/flavorName/play/**/play.txt -
One is not allowed to have any child directories named play - only root directory can have "play" name:
project/my-project/src/flavorName/play/**/play/*.*
Even if the flavor is named play, this would be the root directory: src/play/play - my changes made it look only at the child directories of the subdirectory.
So src/play/play/default-listing.txt is ok, src/play/play/**/play/file.txt is not.
I tested this change with both old Gradle version and new one and in both cases it now passes.
There was a problem hiding this comment.
One is not allowed to have any files named "play"
project/my-project/src/flavorName/play/**/play.txt
Yes to the sentence, no to the example. /play.txt should be fine, /play isn't. On that note, can you add two test cases for this: one allowing play.foo and one disallowing plain play files? Then check that the tests pass pre-PR and we'll be in the clear for behavior changes.
I like the simplicity of your approach, but I'm a little bothered by the performance implications. Previously, we only did validation on changed files (since the input files are incremental). Now, we always run a full directory expansion. I think just because of that'd I'd still prefer the "climb up every file's path and see if its root play dir is contained in the input dir set" approach.
There was a problem hiding this comment.
Thank you - I was so focused on 'file' that I forgot that you could actually have a file play without an extension.
Got actually a little bit misled because GenerateResourcesIntegrationTest fixture does not contain a file named play but only a directory named play (listings/en-US/play/.keep).
This is the reason why the test would start to fail, as there was no file play in the input anymore with newer Gradle version (previously there was a a list of files and also directories, which were then treated asFile). .keep got ignored.
src/main/play/default-language.txt
src/main/play/listings/de-DE/full-description.txt
src/main/play/listings/en-US/full-description.txt
src/main/play/listings/en-US/graphics/phone-screenshots/foo.jpg
src/main/play/listings/en-US/graphics/tablet-screenshots/baz.jpg
src/main/play/listings/en-US/short-description.txt
src/main/play/listings/en-US/title.txt
src/main/play/listings/fr-FR/full-description.txt
src/main/play/listings/fr-FR/graphics/phone-screenshots/bar.jpg
src/main/play/listings/fr-FR/short-description.txt
src/main/play/listings/fr-FR/title.txt
src/main/play/products/sku.json
src/main/play/release-notes/en-US/default.txt
src/main/play/release-notes/fr-FR/default.txt
One is not allowed to have any child directories named play - only root directory can have "play" name:
Is this also valid?
There was a problem hiding this comment.
Is this also valid?
Sorry, which part? Or do you mean in general? If so, then yeah your understanding is correct.
| @@ -380,6 +381,7 @@ class GenerateResourcesIntegrationTest : IntegrationTestBase(), SharedIntegratio | |||
| } | |||
|
|
|||
| @Test | |||
| @Ignore("Previously this test worked because empty catalogs were generated and passed into the task") | |||
There was a problem hiding this comment.
The check for this is here:
This test absolutely has to pass because we rely on it for correctness. The way we pick which resource to use for a particular flavor is dependent on the parent of the play directory being a valid flavor name:
Anyway, off the top of my head I think you can fix name != PLAY_PATH by combining two climbUpTos and if both of them are non-null then you have two play directories. There might be a cleaner way though.
-- play file is invalid -- play.* file is valid -- play child directory is invalid
Signed-off-by: Alex Saveau <[email protected]>
Fixes #1068
There is one test failing -
GenerateResourcesIntegrationTest#'play' file name is not allowed.The thing is that it used to pass due to empty catalogues being passed into
GenerateResourcestask - with the upgrade it's not valid anymore - only actual files are provided.Parameters before:
Parameters after:
@SUPERCILEX I am aware that you do not maintain this project anymore, however could you please tell if the idea behind this test was to verify if no flavor containing the word "play" exists in the project? If so, I'd rather rewrite this check to work differently.