woocommerce-ios

woocommerce-ios copied to clipboard

Description

TL;DR This PR enables dedicated reports for the unit, UI iPhone, and UI iPad tests. It does so by jumping through some hoops when running the build_for_testing lane. I'm not too happy with the setup because it requires a dedicated scheme and test plan, but it does the job for now. Keen for feedback regarding the names and how to document why things are the way they are.

Internal reference: paaHJt-3wn-p2

More details After a lot of trial and error, I finally figured out why we weren't getting Test Analytics reports for the UI tests. In a nutshell, when xcodebuild runs an xctestrun generated by the build-for-testing step, it does not pass the host environment to the tests, instead, the environment the tests have access to is the one of the host that generated the xctestrun file.

This created issues for us because, in order to have multiple suites, we have multiple tokens. My original approach was to export the appropriate one into the value the test plan expects in the CI script, but that didn't work because the tests used the environment from when the xctestrun was generated.

To solve this, I tweaked the setup and:

1. Defined a new scheme that builds both the unit and UI tests.
2. Add a new UI tests test plan with two configuration, one for iPhone and one for iPad. More on this later
3. Update the Fastlane setup to allow test_without_building to run only one configuration in a test plan, thus enabling us to run the tests in iPhone and report to the iPhone Test Analytics suite and on iPad and report to the iPad suite.

About the dedicated test plan for UI tests, UITests-CI.xctestplan

For Test Analytics to read the correct API token, we need to have a test plan - configuration combo that binds the correct environment variable from the host when we run the build-for-testing xcodebuild command (via Fastlane) to the BUILDKITE_ANALYTICS_TOKEN environment variable.

We can achieve this by either having two test plans, one that binds the iPhone suite token and one that binds the iPad one, or with one test plan with two configurations. I went for the one test plan option, thinking I'd be able to define all the common env var bindings in the "Shared Settings" and then only specify the tokens in the configurations.

That didn't work out, as you can see in the commit history. It's possible I made some mistakes in the test plan configuration, but I decided not to investigate further in the interested of finally shipping something that works.

One test plan with two configurations presents a problem: Both configurations run! In the case of UI tests, this is a great waste of time.

One option would be to disable one configuration. I considered that but decided not to do it because there's no way I know of to explain to a future users why the configuration was disabled.

Instead, I decided to create a dedicated test plan with "CI" in its name. My hope is that the name will make it clear to future users that this test plan is not to be used locally.

What do you think?

If this doesn't seem clear, I hope the additional comments in the code will help.

Testing instructions

Verify CI has reports for all three test suites.

Screenshots

---

• [x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary. – N.A.

You can test the changes from this Pull Request by:

• Clicking here or scanning the QR code below to access App Center
• Then installing the build number pr7415-2d18c22 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

Just to make sure I understand the issue correctly, one test configuration can only accept one token as an environment variable, so this PR adds a couple more configurations, so that we have three configurations, each getting its own token, correct?

If that's the case, I have a couple of alternatives in mind without adding another scheme, please let me know if they make sense:

1. Run build_for_testing or run_tests three times, with a different token each time; the goal is getting three xctestrun bundles, for unit test, UI test on iPhone, and UI test on iPad. Then we pick the right one to run on the test_without_bulding lane.
2. Delete the BUILDKITE_ANALYTICS_TOKEN environment from the configuration, and add all three tokens into the configuration with different names. At runtime, after the test is launched, set BUILDKITE_ANALYTICS_TOKEN to the appropriate token based on the type of test and simulator. I did a quick search, it looks like buidlikte test collector self-loads itself, and we'll need to race it to set BUILDKITE_ANALYTICS_TOKEN env var. I'm not sure if this approach will work...

Just to make sure I understand the issue correctly, one test configuration can only accept one token as an environment variable, so this PR adds a couple more configurations, so that we have three configurations, each getting its own token, correct?

Yes. Also, taking a step back, the reason we want three Test Analytics suites (or at least two, unit vs UI) is to keep the reports isolated and have more accurate average test suite duration metrics. Otherwise, the difference between unit and UI tests would hide precious information in the results.

Run build_for_testing or run_tests three times, with a different token each time; the goal is getting three xctestrun bundles...

That's a cool idea that would keep things quite clean on the project configuration side. However, I worry it would end up taking a lot of CI time for something that we can achieve with a bunch of clean-enough extra configuration files.

Delete the BUILDKITE_ANALYTICS_TOKEN environment from the configuration, and add all three tokens into the configuration with different names. At runtime, after the test is launched, set BUILDKITE_ANALYTICS_TOKEN to the appropriate token based on the type of test and simulator...

I had something along those lines going on in an earlier iteration. See here. What I was doing was basically edit the xctestrun to inject the token. That was before I learned how to use environment variables with out two step setup.

The current setup is better that the one I originally had because it doesn't require any extra step in the Fastfile. However, it does come with some configuration overhead.

However, I worry it would end up taking a lot of CI time

I don't think the build time would increase that much. The first run_tests does a clean build, as it does now. The following ones shouldn't take long, since there is no code change. The overhead will be "repackage" time, nothing (or not much code) should be re-compiled. However, I can't say the same for WP, if we use the same approach there.

Sorry I wasn't clear about my second suggestion. I meant setting the BUILDKITE_ANALYTICS_TOKEN env var within test suite, during it's execution. To be exact, setting the token env var should be the first thing we do when test suite is launched, and it has to be done before buildkite test collector loads itself which is quite tricky. IMO, Buildkite test collector should provide an API to let the user instantiate its TestCollector, which would be quite helpful to our use case here.

IMO, having an extra duplicating scheme isn't something very maintainable, since we need to be mindful about this duplication and keep it in-sync with the main scheme. This is potentially something very easy to overlook in a few month's time 🙈

Thank you for the great questions @crazytonyli 👌

I'm keen on iterating on this and getting it "right" (or as less wrong as we can at this point in time) because we can use the learning here to in the other apps. I have early stage PRs in WordPress iOS and Day One iOS that I'm intentionally holding back till we find something we're happy with here.

I don't think the build time would increase that much. The first run_tests does a clean build, as it does now. The following ones shouldn't take long, since there is no code change [...]

I hadn't considered that. It's quite straightforward to check this, anyway. I'll give it a go 👍

IMO, having an extra duplicating scheme isn't something very maintainable, since we need to be mindful about this duplication and keep it in-sync with the main scheme. This is potentially something very easy to overlook in a few month's time 🙈

Very true.

I meant setting the BUILDKITE_ANALYTICS_TOKEN env var within test suite, during it's execution. To be exact, setting the token env var should be the first thing we do when test suite is launched, and it has to be done before buildkite test collector loads itself which is quite tricky.

The only approach I can think of to control this would be to use Quick's beforeSuite(_:), but I don't know whether it would run before the auto-load. Something we can check, though.

---

My next steps:

• [x] Work on an alternative setup that use the multiple run_tests tactic – https://github.com/woocommerce/woocommerce-ios/pull/7466
• [x] Work on an alternative setup based on my earlier version, where I editing the xctestrun to inject the token before running the tests via Fastlane. – https://github.com/woocommerce/woocommerce-ios/pull/7467

@crazytonyli I pushed two alternative versions of the setup, based on our discussion.

Unfortunately, running run_tests three times to generated three xctestrun files each with the correct token is not feasible. Then generated xctestrun does not contain information about the test plan configuration in its name, so it's not possible for us to distinguish between the UI tests iPhone or iPad one.

On the other hand, my original approach of editing the xctestrun in CI just before running it worked.

That approach was originally discarded to bypass the risk of storing the token in the xctestrun. However, we've exhausted the alternative options and, compared to the setup in this PR, injecting the token in the xctestrun file in CI is a much cleaner approach.

• It doesn't require any extra scheme or test plan
• It centralizes the knowledge of the Buildkite Test Analytics environment variables, whereas the other setup require them to be specified in multiple files

I'm relatively relaxed about the risk of the token leaking because:

• The xctestrun files are ignored
• There is a check to make sure the injection only runs in CI
• Those tokens are not mission critical. A bad actor with access to one can only pollute our analytics pages with garbage data

At this point, I think the approach from #7467 is the best one. If you agree, I'd close this PR make that one the official one to adopt. Then, I'll replicate it in WordPress and maybe Day One.

P.S. thank you for your great questions in this PR. I had a feeling the setup was not ideal and you gave me just enough pushback to get to something better. And I know we can keep iterating on it if not ideal still.

@mokagio Thanks for taking the time to experiment with my suggestions. The CI env injection one looks really nice! 👍

Closing in favor of https://github.com/woocommerce/woocommerce-ios/pull/7490.