appium / appium-ios

iOS-specific packages for Appium
https://appium.io
Apache License 2.0
2 stars 1 forks source link

split CI checks into separate jobs; closes #3 #4

Open boneskull opened 3 years ago

boneskull commented 3 years ago

This splits the checks into two buckets: one for tests in ios-simulator which require additional prerequisites in the environment, and the other for everything else (which does not require a special environment other than macOS).

Further, changeset 6163fc2 disables the import/no-unresolved rule. This rule is problematic, because it depends upon the packages having been built (transpiled), which is orthogonal to code linting.

Resolves #3.

boneskull commented 3 years ago

@mykola-mokhnach As per your request, node-simctl has been removed from this monorepo. I've translated the tests in ios-simulator as described in the existing azure templates to just use GitHub's CI (to the best of my ability). We may want to expand the build matrix, for example. There's quite a few different configs we can run on; see the GHA macOS 10.15 environment description. Big Sur is currently unavailable due to demand.

I know you have reasons for wanting to retain the Azure tests--I just wanted to give running it all in GHA a shot, and I'm happy to restore the Azure builds if this proves insufficient.

boneskull commented 3 years ago

I have "formally" asked GitHub for access to the Big Sur environment, so we'll see what they say.

boneskull commented 3 years ago

These tests are running very slowly...

boneskull commented 3 years ago

Here's an example of why we should disable the ESLint rule.

In this case, I'm not sure exactly what's happening (could be a race condition), but the concerns are orthogonal anyhow. You should not need to build other packages to run a lint check. The main field of each package points to a build artifact, which means if you import '@appium/foo', then @appium/foo must have been built to pass the lint check. To be clear: this rule is OK for third party dependencies, but not in our monorepos--it's a chicken-or-egg problem.

I'd recommend addressing this by distributing our packages as dual CJS/ESM. It's possible (likely?) we are using syntax unsupported in our minimum req'd Node.js version, in which case we'd still need to transpile.

boneskull commented 3 years ago

@mykola-mokhnach I've added more checks for Xcode versions running against the default Node.js.

boneskull commented 3 years ago

Note that the ios-simulator E2E tests now depend on the successful execution of the ios-simulator unit tests.

The breakdown of the build matrix is:

  1. All checks (unit, e2e) other than ios-simulator run against supported Node.js versions
  2. The ios-simulator unit tests run
  3. Assuming 2. completes successfully, ios-simulator's e2e tests will run against the default Node.js version (I think it's v14 atm)

Steps 1 and 2 happen concurrently up to whatever GHA's concurrency limit is.

If any job fails, the run will fail-fast.

boneskull commented 3 years ago

This should be ready to merge pending green build.

mykola-mokhnach commented 3 years ago

The breakdown of the build matrix is:

All checks (unit, e2e) other than ios-simulator run against supported Node.js versions The ios-simulator unit tests run Assuming 2. completes successfully, ios-simulator's e2e tests will run against the default Node.js version (I think it's v14 atm)

I would prefer if we could breakdown tests by unit/e2e, so all unit tests are being executed in the first stage while e2e tests are executed in the second stage. I don't see a need to make simulator tests so unique, because other modules also require some special e2e setup and then it would be necessary to change the script again (e.g. the idb one requires facebook idb tool to be installed)

mykola-mokhnach commented 3 years ago

Also, it is hard to parse the tests results stuff in general if it is named common. It would be much easier to see what exatcly failed if these jobs would include corresponding component names

boneskull commented 3 years ago

@mykola-mokhnach

Also, it is hard to parse the tests results stuff in general if it is named common. It would be much easier to see what exatcly failed if these jobs would include corresponding component names

I agree that'd be nice (and was my original idea), but:

a) the status quo in appium/appium is this b) it would significantly increase execution time because it'd need number-of-packages * 3 (node.js versions) VMs to boot up, retrieve node_modules from cache and compile all packages

so I'm inclined to leave it

boneskull commented 3 years ago

I would prefer if we could breakdown tests by unit/e2e, so all unit tests are being executed in the first stage while e2e tests are executed in the second stage. I don't see a need to make simulator tests so unique, because other modules also require some special e2e setup and then it would be necessary to change the script again (e.g. the idb one requires facebook idb tool to be installed)

Hmm, it must be that not all the E2E tests are running, then? Or they are getting skipped?

boneskull commented 3 years ago

yeah they aren't getting run.

mykola-mokhnach commented 3 years ago

Unit tests are fast, so we could easily run all of them for all supported Node versions. It would make sense to run e2e tests for only actually modified packages. And leave other packages for manual execution if needed

boneskull commented 3 years ago

Unit tests are fast, so we could easily run all of them for all supported Node versions.

Yes, we are running unit tests for all supported node versions. However, we're not running these as separate jobs because the setup of the VM takes longer than the unit tests do.

It would make sense to run e2e tests for only actually modified packages. And leave other packages for manual execution if needed

I did consider this, but I think it's something we can work on later--there are interdependencies between these packages, so we'd need to make sure x is run if y is modified.