tests: Move CI from Travis to GitHub Actions. #4383

titanous · 2021-01-05T02:19:30Z

This configuration is equivalent, except that Ubuntu and Java are updated to the latest LTS releases.

gnprice · 2021-01-06T02:59:12Z

Excellent, thanks!

I don't see results from it shown here on this PR thread. Perhaps the workflow needs to exist on a branch in the repo itself? I'll go read some of the docs, and experiment with pushing this commit to a branch here.

gnprice · 2021-01-06T03:04:12Z

... Ha, OK! I:

pushed the exact same commit to a branch within this repo;
made a dupe PR from it, [dup] tests: Move CI from Travis to GitHub Actions. #4384;
and that seems to cause the action to start both on that PR and this one!

In fact it looks like two of them: one for having pushed to a branch, and one for having made a PR from that branch.

I guess because it's the same commit, it's probably showing the same runs' results on both PRs.

titanous · 2021-01-06T03:49:30Z

The (push) runs against the latest ref from the branch, and the (pull_request) runs against a trial merge into the base of the PR. This article explains in more detail.

gnprice

Thanks @titanous ! Small comments below -- otherwise this looks great.

There's a couple of bits this drops from the config, which are good to drop because we haven't been using them but it'd be good to mention in the commit message.

gnprice · 2021-01-06T03:27:57Z

.travis.yml

-
-env:
-  global:
-    - COVERALLS_REPO_TOKEN=4eYQDtWoBJlDz2QkxoQ2UcnmJFcOB7zkv


This gets dropped.

That's fine, because we haven't actually been using Coveralls in this repo for several years.

Noted in commit message.

gnprice · 2021-01-06T03:30:35Z

.travis.yml

-android:
-  components:
-    # all the android components used in the zulip-mobile and it's dependencies
-    - tools
-    - platform-tools
-    - build-tools-28.0.3
-    - android-29  # corresponds to compileSdkVersion
-    - extra-google-m2repository
-    - extra-android-m2repository
-    - extra-google-android-support
-
-  licenses:
-   - 'android-sdk-license-.+'


Huh, all this detail goes away!

As best I can tell from that action's implementation:
https://github.com/android-actions/setup-android/blob/main/src/main.ts
it just installs all versions of the tools:

await exec.exec( sdkManager, ['--include_obsolete', `--sdk_root=${ANDROID_SDK_ROOT}`, 'tools'], {input: acceptBuffer} )

There seems to be no downside, though, because it's quite fast -- only takes 1s to run (and GH Actions doesn't show times at finer granularity than that.) I guess the caching must be pretty great!

Noted in commit message.

gnprice · 2021-01-06T03:32:42Z

.travis.yml

-
-language: android
-
-jdk: oraclejdk8


This gets updated to version 11. That's for the best -- since 0e45fda we've been recommending at least that version for development, so ideally this would have been already updated then.

Noted in commit message.

gnprice · 2021-01-06T03:37:25Z

.travis.yml

@@ -1,46 +0,0 @@
-dist: trusty


Glad to be rid of this line! Don't know if you've already spotted this in the Git history, but here's the reason we've been running our Travis builds on Trusty:

commit 64e02a8
Author: Greg Price [email protected]
Date: Tue Jul 23 14:53:47 2019 -0700

travis: Stay on working Trusty, while "xenial" is really Precise. Fixes #3565. Travis CI is migrating their default build environments from 14.04 Trusty to 16.04 Xenial. That's more than reasonable -- Trusty went EOL a few months ago. But for Android they don't yet have a Xenial image. And when you request a Xenial image for Android... what you actually get turns out to be a *12.04 Precise* image. Which went EOL over two years ago, and which naturally things don't always run on. […] Fix it by explicitly sticking with Trusty, as the least-ancient option available. More details in the issue thread #3565. I also just reported the issue to Travis on their forum: https://travis-ci.community/t/android-builds-default-to-precise-which-is-2-years-eol/4352

It looks like currently ubuntu-latest on GH Actions means 18.04 Bionic, and presumably at some point it'll start meaning 20.04 Focal. Sounds good.

Noted in commit message.

gnprice · 2021-01-06T03:38:45Z

.travis.yml

-notifications:
-  email: false
-  webhooks:
-    urls:
-      - https://zulip.org/zulipbot/travis
-    on_success: always
-    on_failure: always


This gets dropped.

That's fine, because we don't have zulipbot actually doing anything with these notifications, and haven't for years.

Noted in commit message.

gnprice · 2021-01-06T03:42:31Z

.github/workflows/ci.yml

+    - name: Run yarn install
+      uses: bahmutov/npm-install@v1


Huh, does this actually -- huh, I guess this does indeed use Yarn!

Probably deserves a one-line comment to acknowledge that yes, the name sounds like it does npm install, but it actually does yarn install.

Noted in comment.

gnprice · 2021-01-06T03:50:25Z

.travis.yml

-  # See the mention of `inotify` in docs/.  If we attempt assembleRelease,
-  # then without this line the JS bundle step fails with a baffling ENOSPC
-  # error, even though there's tons of free space on the filesystem.
-  - echo fs.inotify.max_user_watches=524288 | sudo tee -a /etc/sysctl.conf && sudo sysctl -p


This bit goes away. Here's the commit that added it (and nothing else):

commit 65882db
Author: Greg Price [email protected]
Date: Wed Nov 14 17:59:19 2018 -0800

travis: Add incantation to fix release build (but don't actually do it.) With this rather arcane, but harmless, command, the mysterious ENOSPC error caused in Travis by switching to the release build no longer happens, and the build completes successfully. On the downside, the build is about 4 minutes longer -- over 50% longer -- than with the debug build. That JS bundle step is *slow* in Travis. So although it works, it's not worth the time penalty to do on every PR; we'll leave it on debug. We may sometimes want to turn it on in the future, though. So leave this here to prevent repeating this debugging session.

I think that's still a good reason to keep it in. In fact now that we'll be on GH Actions, I think it becomes more likely that we'll try re-adding a release build. We'd do it as a separate job running in parallel, I think.

Added a comment with this info. Given that it will run in a separate job, I don't think we need to actually run the command in the current config.

Sure, makes sense.

gnprice · 2021-01-06T03:55:38Z

.github/workflows/ci.yml

+      with:
+        node-version: '12'
+        check-latest: true
+


nit: whitespace at end of line

Hmm, it seems like it wasn't. But easy for me to fix before merge 🙂

gnprice · 2021-01-06T04:19:39Z

The (push) runs against the latest ref from the branch, and the (pull_request) runs against a trial merge into the base of the PR. This article explains in more detail.

Ah, good to know.

In our usual workflow we don't push to branches here -- we each push feature branches to our respective personal forks and send those as PRs. So I think that means that typically push won't run on a PR, only pull_request, and push will run when there's a push to master (the only branch we routinely push to.)

gnprice · 2021-01-06T04:24:34Z

After the small changes discussed above and after we merge this, If you're interested, I think there's a couple of followups this opens up that would potentially make the CI significantly faster and nicer than it's ever previously been:

Cache node_modules, to save a good chunk of the time.
Split into several jobs that run in parallel (and call tools/test with specific suite names to do different parts of the tests), to make it faster still.
- Probably these would all depend on a common initial job that does yarn install and saves to the aforementioned cache.
Persuade Flow to stop spewing "Please wait. Server is initializing (parsed files 30000)" messages, and persuade Jest and Flow and other tools to use color.
- This would probably involve adding an option like --ci to tools/test and passing it here. Then each suite in tools/test would pass the appropriate options to tell the respective underlying tool what to do.
- It'd replace the TERM=dumb, which is a bit of a crude solution explained in 6614384 for getting Gradle to stop spewing something a lot like that Flow spew. That TERM=dumb is likely the reason Jest isn't showing color; and on the other hand Flow didn't take the hint.
Get the fancy log grouping like some of the upstream actions do, with ⯈/⯆ expandos. It looks like they do this with calls like core.startGroup('Running foo'), using the NPM package @actions/core. ... And looking at that function's implementation (1, 2), I think that boils down to echo "::group::Running foo". (With a bit of percent-encoding in case the message contains newlines.)
- This might not be relevant if we end up splitting the tools/test run completely into separate jobs. Or for that matter we could split it into separate steps, which might also make this irrelevant. Well, it was entertaining for me to dig into just now. 🙂

titanous · 2021-01-06T17:25:21Z

Thanks for the detailed review!

Cache node_modules, to save a good chunk of the time.

The npm-install action already does this.

The configuration is similar, with these changes: - Update from Java 8 to Java 11, the recommended version for development - Update from Ubuntu 14.04 to 18.04 (and eventually 20.04 automatically) - Rely on the setup-android action to provide all versions of the tools - Remove unused Zulipbot webhook - Remove unused Coveralls token Fixes: zulip#4174

gnprice · 2021-01-06T18:27:25Z

And merged 🎉 -- thanks!

I made some small whitespace changes, and small edits to the commit message -- mostly just wrapping it to within 70 columns.

gnprice · 2021-01-06T18:32:22Z

Cache node_modules, to save a good chunk of the time.

The npm-install action already does this.

Hmm! That'd make sense, but... it looks like that step took 1m29s on the run after your update. That's about as long as it was on the previous revision (1m35s), and seems like long enough to install the node_modules from scratch.

Maybe the cache will only work after there's a version in master? I guess we'll find out. If it continues to not benefit from caching, then I guess we'll want to debug.

gnprice mentioned this pull request Jan 6, 2021

[dup] tests: Move CI from Travis to GitHub Actions. #4384

Closed

gnprice reviewed Jan 6, 2021

View reviewed changes

titanous force-pushed the github-actions branch from b279912 to 87ab409 Compare January 6, 2021 17:22

gnprice force-pushed the github-actions branch from 87ab409 to 7dfa1c2 Compare January 6, 2021 18:25

gnprice merged commit 7dfa1c2 into zulip:master Jan 6, 2021

titanous deleted the github-actions branch January 6, 2021 21:22


		language: android

		jdk: oraclejdk8

Uh oh!

Uh oh!

tests: Move CI from Travis to GitHub Actions. #4383

tests: Move CI from Travis to GitHub Actions. #4383

Uh oh!

Conversation

titanous commented Jan 5, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

titanous commented Jan 6, 2021

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titanous Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titanous Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titanous Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titanous Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

titanous Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

titanous commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

gnprice commented Jan 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

titanous Jan 6, 2021 •

edited

Loading

titanous Jan 6, 2021 •

edited

Loading

titanous Jan 6, 2021 •

edited

Loading

titanous Jan 6, 2021 •

edited

Loading

titanous Jan 6, 2021 •

edited

Loading