ref(appconnect) Add tests for the API usage #29780
Conversation
This adds tests which can run against both the live API as well as with mocked responses. The mocked responses still use the entire request code handling so everything is covered by tests. This does one functionality change: our two specific test builds are by now "exired" on App Store Connect as builds on test flight expire after 30 days. Expiring a build means they become unavaible to newly install it onto a phone participating in the test group. However the build and dSYMs remain available, existing phones could keep using the build after all. So arguably we shouldn't have been skipping expired builds anyway. Another motivation to not skip expired builds is because for a long running project those builds are all downloaded anyway before they expire. An argument against using expired builds is that the list of builds that we iterate on every check will grow much faster which may require us to be smarter about not re-downloading information from the API for builds we already know.
| "&limit=200" | ||
| # include related AppStore/PreRelease versions with the response as well as | ||
| # buildBundles which contains metadata on the debug resources (dSYMs) | ||
| "&include=appStoreVersion,preReleaseVersion,buildBundles" |
There was a problem hiding this comment.
nit: why the reordering here?
There was a problem hiding this comment.
Yeah, it's silly:
- Apple re-orders these in this order in the
selfandnextlinks in API responses - The tests sets up
responseswith the URLs fromselfandnextfrom the recorded responses. responsesis smart enough to understand query parameters can be re-ordered when mapping requests to responses, but it does not know these arguments to a parameter are equivalent.
So the simplest thing was to align this with how apple builds the response
| # and builds that have not expired yet | ||
| "&filter[expired]=false" |
There was a problem hiding this comment.
let's just delete this if we're no longer using it
| key_id=data["key_id"], issuer_id=data["issuer_id"], key=data["private_key"] | ||
| ) | ||
| else: | ||
| # NOTE: This key has been generated for these test. |
There was a problem hiding this comment.
what does this mean exactly? where are the contents of the not-live (dead? undead, unlive?) private key sourced from?
There was a problem hiding this comment.
I mean the key has been freshly generated and is just a random key, it will not be recognised anywhere and is thus safe to commit to our source code.
I could entirely remove the comment if you prefer.
There was a problem hiding this comment.
thanks for the explanation! i think keeping the comment is good although i'd love to see it rephrased to something more along the lines of the first sentence of your reply, e.g.
# NOTE: This key has been generated outside of App Store Connect for the sole purpose of being used in this test. The key is not associated with any account on App Store Connect, and therefore is unable to access any live data.the original comment leaves a lot of details out, namely that it's not associated with anything and therefore is essentially a throwaway key that can't do anything dangerous.
| "&limit[buildBundles]=50" | ||
| "&sort=-uploadedDate" | ||
| "&filter[processingState]=VALID" | ||
| # "&filter[expired]=false" |
There was a problem hiding this comment.
similarly, this could probably be removed
relaxolotl
left a comment
relaxolotl
left a comment
There was a problem hiding this comment.
the changes look good, although it's currently not too clear to me if we have any concrete plans to actually use the live testing scenario.
my understanding is that we don't want to run it in CI to avoid hammering apple's servers and getting ourselves rate limited. this leaves just manual runs where we deliberately remember to create an apikey.json. given how often we were running the iTunes API tests, it seems unlikely that these will ever be run live.
as a minor request for the future, i would like to see functional changes like the one involving expired builds in this PR broken out into their own dedicated PRs. the functional changes will now be bundled into the same commit as the tests, which makes reverting a little awkward. not only that, but it makes commit spelunking a little awkward if you're investigating anything that's related to the functional change: the title says one thing while you're investigating another thing.
i realize that breaking these out into their own PRs does introduce more logistical work in trying to wrangle people to review more than one PR, making sure they get merged in the right order, possibly rebasing off of master, etc. i do think it's worth it in this specific scenario though.
| def write_paged_build_response( | ||
| self, api_credentials: appstore_connect.AppConnectCredentials, app_id: str | ||
| ) -> None: | ||
| """Use this function to create the ``pages_data.jons`` fixture data. |
This adds tests which can run against both the live API as well as
with mocked responses. The mocked responses still use the entire
request code handling so everything is covered by tests.
This does one functionality change: our two specific test builds are
by now "exired" on App Store Connect as builds on test flight expire
after 30 days. Expiring a build means they become unavaible to
newly install it onto a phone participating in the test group.
However the build and dSYMs remain available, existing phones could
keep using the build after all. So arguably we shouldn't have been
skipping expired builds anyway. Another motivation to not skip
expired builds is because for a long running project those builds are
all downloaded anyway before they expire.
An argument against using expired builds is that the list of builds
that we iterate on every check will grow much faster which may require
us to be smarter about not re-downloading information from the API for
builds we already know.