Conversation
|
View this pull request in Lando to land it once approved. |
|
No new issues detected. This pull request is 🆗 |
498d1e8 to
50ba977
Compare
|
No new issues detected. This pull request is 🆗 |
50ba977 to
ee7547d
Compare
segunfamisa
left a comment
segunfamisa
left a comment
There was a problem hiding this comment.
Thanks Polly.
Please, could you help with a bit of explanation for my mind to grok the idea here?
I am approving nonetheless, so that I'm not blocking the patch.
| // Web extensions that only the UI tests install. | ||
| // They have to live in the app under test rather than the test apk. | ||
| // Added to the debug and other test builds, so they never reach a shipping build. | ||
| debug { | ||
| assets.srcDirs += ['src/uiTest/assets'] | ||
| } | ||
| if (project.hasProperty("testBuildType")) { | ||
| named(project.property("testBuildType")) { | ||
| assets.srcDirs += ['src/uiTest/assets'] | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure I fully understand why we need a new source set for this - and more importantly, I think we may need more info about why and how this change helps. Especially for 2028 Segun who's 100% gonna forget ever reading this PR 😂
There was a problem hiding this comment.
I think what I'm struggling with is: I am sure there's a reason, but it's not obvious to me why androidTest is not sufficient or desirable for this use-case.
There was a problem hiding this comment.
oh that's fair, thanks! i guess i could put them in androidTest/assets as a top level thing, not sure why i didn't think of that.
I was thinking everything existing was bucketed by build variant but i missed this dir.
i will give it a go!
There was a problem hiding this comment.
I don't know if it would work or not btw - I was just wondering tbh.
I think if we are running androidTest<Variant>, it should still fallback and pick resources and stuff from androidTest
There was a problem hiding this comment.
i've pushed a change based on your suggestion - do you prefer this?
i don't really think there is a perfect place for these assets tbh: they are a bit unusual in that they are assets required only by the ui tests, and yet they need to live in the app apk to be picked up as web extensions.
There was a problem hiding this comment.
reworded the comment a bit too, lmk if that makes sense. also happy to chat more about it :)
There was a problem hiding this comment.
(sorry, push failed the first time for some reason! should be good now!)
Previously this relied on assets only visible to debug builds, so i've moved these to a place where other testable build types can also use them (eg. nightly).
ee7547d to
ca390ea
Compare
This branch has not been deployed
2 participants
Previously this relied on assets only visible to debug builds, so i've moved these to a place where other testable builds can also use them.
Try is running here
Lando: link
Bugzilla: bug 2073095
🚫 This pull request has 1 blocker.