Handle multiple APKs installable for an Installable Build #79
Conversation
|
👋 @AliSoftware ! I wonder, can you share some testing instructions to verify if/how the changes here work? I'm not familiar with the Peril integration and I wonder how can I go about reviewing this PR. Thanks! |
|
The testing instructions are a bit tricky for Peril, because it's written in TypeScript… so if you want to run the unit tests (which I updated in that PR), you need to have But this is something that the CI already does (running unit tests), so if the CI is green that means those passed and no need to test locally. I don't think we can test much more (e.g testing the rule on an actual PR) easily given this is a bot that acts on all our repos and only uses the config and rule files in |
|
Thanks for elaborating Olivier! I'll give the diff a visual look and probably optimistically give the go ahead if nothing captures my attention :) |
| @@ -151,4 +151,29 @@ describe("installable build handling", () => { | |||
|
|
|||
| expectComment(webhook, `You can test the changes on this Pull Request by downloading the APK [here](${mockedArtifacts[0].url}).`) | |||
| }) | |||
|
|
|||
| it("Posts a download comment linking to multiple APKs", async () => { | |||
There was a problem hiding this comment.
Note that this unit test is just a shameless copy/paste of the previous unit test (lines 133-153) for single-APK case, adjusted to have a mockedArtifacts containing multiple APKs and an adjusted expectation.
There was a problem hiding this comment.
LGTM @AliSoftware ! I only left a very minor comment, otherwise looks good to me.
While at it I verified that tests do run correct locally as well 👍
Thanks @hypest
The link in the Peril comment for the installable WPAndroid build (e.g wordpress-mobile/WordPress-Android#14936 (comment)) is actually pointing to the Jetpack app.
We're able to download the apk if we visit the artifacts page anyway, but it's nicer to have Peril provide the link for all installable APKs not just the first one.
\cc @hypest