Skip to content

Conversation

@kgritesh
Copy link
Contributor

@kgritesh kgritesh commented Oct 28, 2019

This change will integrate Pirlo CLI with the Travis CI system. The Pirlo CLI script will execute a set of pre-defined test cases on real devices in the cloud. The CLI script will output the link to the latest test run in the console.
For your reference, the UI tests can be viewed at https://app.pirlo.io/tests?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D and results for each build is available at https://app.pirlo.io/runs?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D

@kgritesh kgritesh force-pushed the feat/enable-pirlo-tests branch 2 times, most recently from 7c7403d to 12f9eca Compare October 31, 2019 19:27
@kgritesh kgritesh changed the title feat: Enable pirlo testing Enable end to end testing via Pirlo Oct 31, 2019
@kgritesh kgritesh force-pushed the feat/enable-pirlo-tests branch from 12f9eca to 6513d8f Compare November 1, 2019 16:30
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looking forward to having this set up.

I have a couple of questions below for before merge, and one bit of (non-blocking) feedback on the distribution flow.

There are also some aspects of the shell-scripting style that differ from our preferred style, but that's all easy for me to fix up when/after merging.

.travis.yml Outdated

script:
- TERM=dumb tools/test --full
- TERM=dumb travis_wait tools/test --full
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting -- what's this bit do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to ensure that this command keeps on running for 20 minutes even if it doesn't print any output. More information can be found here https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received


set -e
echo "Downloading pirlo-cli tool"
curl https://pirlo-downloads.s3-us-west-1.amazonaws.com/pirlo-cli/pirlo-cli -o pirlo-cli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now for getting started, but one thing that would streamline things is to have the CLI tool available as a package on NPM, so that we could add it to our dependencies in package.json and it'd get installed by yarn install.

In particular that'd mean that once it's downloaded, it doesn't have to be re-downloaded unless a new version is specified. Doesn't much matter in CI, but that would make it a lot smoother to use a script like this interactively.

(Despite the name, there's no need for JS let alone Node to be involved in an NPM package; it basically amounts to a tarball with a name and a version number and a few other bits of metadata, all stuffed into a JSON file at a well-known name inside the tarball. Also it's shockingly easy to upload to NPM.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. Will definitely add this to our development roadmap and implement it as soon as possible.

tools/test Outdated

if [ -z "$suites" ]; then
suites=(android flow lint jest prettier)
suites=(android flow lint jest prettier functional)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll want to leave it out of these defaults (for when the test script is run without arguments), so you say tools/test functional when you want to run it. Then we can just say that in the Travis config to make it run in CI.

That takes it out of the hot loop in normal development (edit, run tests, edit, run tests, etc.) -- we try to keep that very fast, like a few seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure makes complete sense. I have now removed functional test suite from default test suite.


echo "Initiating a new automated test run."

./pirlo-cli run -k jNmWe3iFhWjvSy_SCjlvLsCWgfw= android/app/build/outputs/apk/release/app-release.apk No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this token mean -- is it a form of auth, and if so what's the scope of it?

One thing we can do if there needs to be a secret involved (because this is a public repo) is IIRC I can put it in Travis config so it gets passed as an environment variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is an auth token with a limited scope to be used by CI system. The auth token can only be used for initiating an automated test run and getting its status. We also agree its better idea to add it as an environment variable in your travis config. You can set an environment variable name PIRLO_API_KEY and then we don't need to pass it as cli argument.

@gnprice
Copy link
Member

gnprice commented Nov 2, 2019

Hmm, also it looks like the CI build failed because Travis timed it out at 20 minutes. Our builds in master are 9-10 minutes:
https://travis-ci.org/zulip/zulip-mobile/builds
The release build the new test path made says it took just over 6m, and I guess the actual test took more than 4-5m after that.

One thing this probably means is that we'll want to run this in parallel with our existing tests, rather than in sequence. I'm not sure that's easy to do in Travis, hmm.

From email notifications, I see you're already watching the repo and triggering builds based on it. What do you think about sending a notification directly to GitHub too? I believe this is the API that Travis and its various competitors and the like use:
https://developer.github.com/v3/repos/statuses/
That'd save some indirections -- your results would show up as a line directly in the GitHub UI (just like Travis does), and it'd avoid having to do anything on Travis.

In any case that needn't block us from getting the script merged so we can run it ad hoc, which would be a good step forward. I'm curious about how the keys or tokens work, though, as mentioned above.

@kgritesh kgritesh force-pushed the feat/enable-pirlo-tests branch from 6513d8f to 8f883e2 Compare November 2, 2019 08:52
@kgritesh
Copy link
Contributor Author

kgritesh commented Nov 2, 2019

Hmm, also it looks like the CI build failed because Travis timed it out at 20 minutes. Our builds in master are 9-10 minutes:
https://travis-ci.org/zulip/zulip-mobile/builds
The release build the new test path made says it took just over 6m, and I guess the actual test took more than 4-5m after that.

One thing this probably means is that we'll want to run this in parallel with our existing tests, rather than in sequence. I'm not sure that's easy to do in Travis, hmm.

From email notifications, I see you're already watching the repo and triggering builds based on it. What do you think about sending a notification directly to GitHub too? I believe this is the API that Travis and its various competitors and the like use:
https://developer.github.com/v3/repos/statuses/
That'd save some indirections -- your results would show up as a line directly in the GitHub UI (just like Travis does), and it'd avoid having to do anything on Travis.

In any case that needn't block us from getting the script merged so we can run it ad hoc, which would be a good step forward. I'm curious about how the keys or tokens work, though, as mentioned above.

I have updated the PR to now ensure that functional test and your remaining test suite can run in parallel. The latest build is now passing and is not timing out. We believe the main power of the platform is to be integrated with travis. As mentioned above, adding auth token as an environment variable to your build settings works out of the box and we can remove the auth token as soon as you have added it to travis build settings.

@vinayjain404
Copy link
Contributor

@gnprice what are good next steps for this PR?

happy to make changes further if helpful.

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

Hello! Sorry I've let this linger for a while.

I just rebased this branch, and then tried running it. The script seemed to work, except I couldn't actually get the results:


I ran tools/test functional. The output ended with:

Initiating a new automated test run.
2019/12/09 17:22:54 Uploaded APK android/app/build/outputs/apk/release/app-release.apk
2019/12/09 17:22:54 Initialized a test run with ID: 1c54f2f8-2f82-47e0-bfe6-50a125f7add4
Running tests...▝ 2019/12/09 17:27:14 Test Run 1c54f2f8-2f82-47e0-bfe6-50a125f7add4 successful. Results: https://app.pirlo.io/run/1c54f2f8-2f82-47e0-bfe6-50a125f7add4/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D
Passed!

But that link doesn't work. I get a page with an empty table that says "Sorry, no matching records found", and a red error banner at the bottom that says "We were not able to fetch all your results. Please try again later."

I also tried just browsing the app, at pages like https://app.pirlo.io/tests and https://app.pirlo.io/runs , and got similar results.


Then, trying the "gear"/"settings" icon in the left nav on the webapp, I got https://app.pirlo.io/integrations which shows me an API key. I guess that API key is for my account, and the one in the PR is for another one, which makes sense.

Maybe that explains why I couldn't see the results, though? So I tried putting my API key, copied from that page, in the script instead of the one that's there.

This time, it quickly failed like so:

$ time tools/pirlo-tests.sh 
Downloading pirlo-cli tool
Initiating a new automated test run.
2019/12/09 17:29:15 Invalid status code while performing API call: 401 Unauthorized

time: 1.679s wall (0.128s u, 0.075s s)

So I guess my API key isn't valid? In any case, that didn't fix the original problem that I can't see the results of a run.

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

Same story with the results from the Travis run on this PR -- that's https://travis-ci.org/zulip/zulip-mobile/jobs/606361300 and the results end with:

Initiating a new automated test run.
2019/11/02 09:03:46 Uploaded APK android/app/build/outputs/apk/release/app-release.apk
2019/11/02 09:03:46 Initialized a test run with ID: 9dc20183-cce8-40cf-a8bc-a497f0a6e41d
RuRuRunning tests...▗ 2019/11/02 09:07:07 Test Run 9dc20183-cce8-40cf-a8bc-a497f0a6e41d successful. Results: https://app.pirlo.io/run/9dc20183-cce8-40cf-a8bc-a497f0a6e41d/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D
Passed!

But that link:
https://app.pirlo.io/run/9dc20183-cce8-40cf-a8bc-a497f0a6e41d/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D
doesn't work for me -- it shows no results, and with a red error banner at the bottom.

I also pushed the rebased branch to my own fork, so Travis would run there:
https://travis-ci.org/gnprice/zulip-mobile/jobs/622972652
Same result.

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

A few smaller comments:

  • In the CLI output, the "Results: $link" should be a separate line from the "Test Run $long_id successful." Both lines are long; you can see in the quoted output above that it gets hard to read, and hard to see the link, when they're combined.

  • There should also be a newline after the "Running tests... $spinner" is complete, so the "successful" line isn't squashed after it.

  • That parallelization solution works well enough, thanks!

    I am dubious that it's wise to pin your strategy so firmly on Travis:

    We believe the main power of the platform is to be integrated with travis.

    as it's one of many CI services, and not one of the better ones. But shrug that doesn't matter for us.

  • Hmm, this travis_wait tool seems to cause a significant regression, though, in the UX of using our test suite in Travis. Normally if a Travis job is still running, and you look at it on the Travis website, you get a live stream of the output it's showing. That can be very helpful for seeing the progress of a build -- both to guess how much longer before it's done, and to see if it's already gotten past a test that you thought there might be a problem at. (For example, a test that was failing and you've been trying to fix.)

    But with this PR branch, I'm not seeing that. As soon as it gets to the step $ TERM=dumb travis_wait tools/test --full ${TEST_SUITE}, it just hangs there for however long it takes until the whole thing is done.

    So, that's no good. Certainly not when applied to our existing set of test suites.

    It'd be OK to have the travis_wait workaround for just the new test suite -- then it's not regressing anything.

    But also, it sounds like the trouble happens here only if your CLI produces no output for over 10 minutes. It might be a good idea to take care of that inside the CLI tool.

    And: in the runs I've tried, our Pirlo suite takes a little over 4 minutes. So is there a reason we'd need this travis_wait wrapper in any case?

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

Oh, one more:

I don't see at https://app.pirlo.io/integrations (which shows me my API key) an option to invalidate it, and get a new one.

That's an important security feature: any API key needs a way to revoke it.

Ideally it would be possible to generate a new key, and have both work for a short time in order to enable a smooth rollover. But for a service like this one that isn't especially critical (it's fine if a few builds fail in the minutes or hour or so around changing a key), and it's a bit more complicated on your side. The simplest possible thing -- I hit a button, the old key is gone, there's a new one -- would work fine.

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

(Thanks again for working on this!)

@vinayjain404
Copy link
Contributor

@gnprice Thanks for the response. We are looking into this and will get back to you shortly.

Are you logged into Pirlo using greg@zulipchat.com? Could you try to logout and access the link https://app.pirlo.io/run/1c54f2f8-2f82-47e0-bfe6-50a125f7add4/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D?

Add a functional test suite to `tools/test` that runs e2e test cases recorded on pirlo.io. Also ensure that functional test run in parallel to other test suites.
@kgritesh kgritesh force-pushed the feat/enable-pirlo-tests branch from 8f883e2 to d6fe654 Compare December 10, 2019 19:33
@vinayjain404
Copy link
Contributor

@gnprice We have updated the PR with the suggested changes except for the API security fix which will be tackle in our upcoming sprint.

To help answers you posted above

1/ Why could you not see results?
We changed the format of our JWT token to support anonymous access for Zulip. You an existing JWT token from a prior version and hence that broke authentication for the APIs.

2/ Why are you seeing differents API keys in "Integration settings" and this PR?
We have two API keys provisioned for clients. One API key is for CI systems that can be seen in the Integration settings page and another one for Anonymous access.

3/ Do we need travis_wait?
We don't need it and have removed it since we are now sending output from the CLI script continually.

4/ Why are tied to Travis CI?
Good point. We would like to integrate with CI systems for continual testing to benefit from our platform.

One benefit of working inside a CI system directly is the APK building portion is handled already for us. We can move towards consuming Github events directly but that would involve creating a system to build APK which is easy but not trivial.

Can you verify the functionality and let us know if we can move forward with the integration?

@gnprice
Copy link
Member

gnprice commented Dec 10, 2019

@vinayjain404 Thanks for the updates!

Are you logged into Pirlo using greg@zulipchat.com? Could you try to logout and access the link https://app.pirlo.io/run/1c54f2f8-2f82-47e0-bfe6-50a125f7add4/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D?

Seems to work!

When I followed that link a few minutes ago, I found that it actually already worked; same for the other link https://app.pirlo.io/run/9dc20183-cce8-40cf-a8bc-a497f0a6e41d/results?authToken=9mf3-dtyE1lMeDFFoRjm391oY-0%3D . Also the bar at the top of the page said "Welcome anonymous".

Then I went to just app.pirlo.io , and got a login page. I logged in, and https://app.pirlo.io/tests shows one run, from "a month ago".

Those two links to specific runs' results still work too. I find that

I'll rerun the suite and confirm that all looks good.

@gnprice
Copy link
Member

gnprice commented Dec 11, 2019

  • OK, one thing I find: could you please make the CLI not print out multiple lines of "Running tests..." per second. 😛 That is pretty annoying when running it directly in a terminal -- which is something I'd expect to do repeatedly any time we're debugging an issue that Pirlo finds for us, or developing changes to the Pirlo suite itself.

    • I realize that was probably the simplest possible change to fix the "no output for 10 minutes" issue, but I'm sure you can find a good balance. For example, one new line per 60 seconds would thoroughly handle "no output for 10 minutes", but keep it from flooding the terminal history.

    • (Not a blocker for merge, though.)

  • I just spotted this bit of the diff:

 env:
   global:
-    - ANDROID_HOME=${TRAVIS_BUILD_DIR}/android-sdk
-    - PATH=${ANDROID_HOME}/:${ANDROID_HOME}/tools/:${ANDROID_HOME}/platform-tools/:${PATH}

Was there a particular reason for deleting those lines?

I don't know of a real reason they should be there -- they seem like legacy cruft -- so I don't mind taking them out. But I'd be curious to know if there was a particular problem you ran into that caused you to try taking them out.

  • We have two API keys provisioned for clients. One API key is for CI systems that can be seen in the Integration settings page and another one for Anonymous access.

    Hmm, I see. Does that mean that both of the two API keys are associated with my account? What's the difference between the capabilities / permissions that each key has?

    If both API keys are associated with my account, it'd be good to have a way to see, and rotate, both of them in the web UI.

  • One benefit of working inside a CI system directly is the APK building portion is handled already for us. We can move towards consuming Github events directly but that would involve creating a system to build APK which is easy but not trivial.

    Mmm, I see. That certainly makes sense.


Build is still running, because Travis is flaky/slow. I've kicked off a new attempt. (It had a network failure downloading Node... so nvm decided to build Node from source.)

Provided that works fine, I expect to merge this today. Thanks again!

@gnprice
Copy link
Member

gnprice commented Dec 11, 2019

OK, and that indeed worked! So, about to merge.

One more bit of UI feedback:

  • It'd be helpful for the CLI to print the results link at the beginning -- as soon as it exists, anyway -- rather than at the end.

    That way, when it's still going I can follow it and go see results. This is especially valuable when debugging or developing a change, as it can shorten the iteration loop a lot.

    It looks like the results for a given device aren't available in the webapp until the run on that device is complete. It'd be extra helpful if you can make those results show up live, step by step -- but I can imagine that may require more infra work.

    But even without infra changes, it can already be the case that one device finishes well before the whole test run is complete, so its results are already available on the web. (I saw that just now, when I experimented with a change that broke login, to check that the test suite failed.) Which means that just printing the link early could be quite helpful.

@vinayjain404
Copy link
Contributor

@gnprice Will make the suggested changes in the CLI and follow up on other questions.

@gnprice
Copy link
Member

gnprice commented Dec 11, 2019

OK -- merged! Pushed the following commits:
788758b tests: Enable end to end testing via Pirlo
0637187 tests: Make small fixes to Pirlo test code.
917f47c tests: Rename Pirlo runner script: no .sh
ac6a96b tests: Rename "functional" suite more specifically to "pirlo".

The first is the one from this PR, with small adjustments to the commit message. The rest are some small commits of mine on top.

(Usually I push back to the PR branch just before merge, which allows GitHub to draw the connection between the commits pushed and the PR, and show the PR as merged. I wasn't able to do so for this branch, so GitHub will see it as closed; the difference isn't a meaningful one.)

@gnprice gnprice closed this Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants