Closed Bug 907177 Opened 7 years ago Closed 6 years ago

[Clock] Add integration tests

Categories

(Firefox OS Graveyard :: Gaia::Clock, defect)

x86_64
Linux
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
tracking-b2g backlog

People

(Reporter: jugglinmike, Assigned: jugglinmike)

References

Details

(Whiteboard: [priority])

Attachments

(4 files, 3 obsolete files)

The Clock application's behavior is not currently backed by integration tests. Write a suite of simple tests that asserts basic behavior, such as the visibility of key UI elements and the creation of alarms.
Hi Ian!

These tests assert basic high-level functionality. I would like to increase the coverage, but I think we can do that incrementally on an as-needed basis.

You can run these with `make test-integration`.
Attachment #792826 - Flags: review?(iliu)
Comment on attachment 792826 [details] [diff] [review]
Bug-907177-add-integration-tests.patch

>From 363ed0c09015a426746caa30c830f1e513bac4bd Mon Sep 17 00:00:00 2001
>From: Mike Pennisi <mike@mikepennisi.com>
>Date: Fri, 2 Aug 2013 12:46:42 -0400
>Subject: [PATCH] Bug 907177 - [Clock] Add integration tests r=ianliu
>
>---
> apps/clock/test/marionette/clock_view_test.js |  154 +++++++++++++++++++++++++
> 1 file changed, 154 insertions(+)
> create mode 100644 apps/clock/test/marionette/clock_view_test.js
>
>diff --git a/apps/clock/test/marionette/clock_view_test.js b/apps/clock/test/marionette/clock_view_test.js
>new file mode 100644
>index 0000000..e6ed8d2
>--- /dev/null
>+++ b/apps/clock/test/marionette/clock_view_test.js
>@@ -0,0 +1,154 @@
>+marionette('Clock', function() {
>+  var CLOCK_ORIGIN = 'app://clock.gaiamobile.org';
>+  var assert = require('assert');
>+  var client = marionette.client();
>+  var selectors = {
>+    analogClock: '#analog-clock',
>+    digitalClock: '#digital-clock',
>+    alarmFormBtn: '#alarm-new',
>+    alarmForm: '#alarm',
>+    alarmFormCloseBtn: '#alarm-close',
>+    alarmCreateBtn: '#alarm-done',
>+    alarmNameInput: '#edit-alarm [name="alarm.label"]',
>+    timeInput: '#time-select',
>+    alarmList: '#alarms',
>+    alarmListItem: '.alarm-cell',
>+    countdownBanner: '#banner-countdown'
>+  };
>+
>+  function padZeros(val) {
>+    val = String(val);
>+    while (val.length < 2) {
>+      val = '0' + val;
>+    }
>+    return val;
>+  }
>+
>+  // Set the value of a given input element. Encapsulates logic for setting
>+  // "time" and "date" input elements (which is currently unsupported by
>+  // Marionette).
>+  function setValue(element, value) {
>+    var type = element.getAttribute('type');
>+    if (value instanceof Date) {
>+      if (type === 'time') {
>+        value = [value.getHours(), value.getMinutes(), value.getSeconds()]
>+          .map(padZeros).join(':');
>+      } else {
>+        value = [values.date.getMonth() + 1, date.getDay(), date.getFullYear()]
Some nits here.. It should be as following:
value = [value.getMonth() + 1, value.getDay(), value.getFullYear()]

Since type of input tag is "time", the test won't run into the else scope.
So, it's not making the integration test failed.
But I would like to comment for revised a correct patch.

>+          .map(padZeros).join('-');
>+      }
>+    }
>+    element.client.executeScript(function(elem, value) {
>+      elem.value = value;
>+    }, [element, value]);
>+  }
>+
>-- 
>1.7.9.5
>
Attachment #792826 - Flags: review?(iliu) → review+
Note: The result of integration test.
========================================================================
$ ./bin/gaia-marionette ./apps/clock/test/marionette/clock_view_test.js
./node_modules/.bin/mozilla-download --verbose --product b2g b2g
directory exists b2g exiting
npm WARN package.json marionette-client@0.13.3 No repository field.
npm WARN package.json marionette-profile-builder@0.0.2 No repository field.
npm WARN package.json marionette-js-runner@0.1.0 No repository field.

  ․․․

  3 passing (19 seconds)
========================================================================

The test works for me. And r=me if there no small nits.

Hi Mike,

Could you please file a pull request? That will be more convenient for adding comment and reviewing process on Github. Thanks for your effort. This is my first reviewing work for "Integration Tests" patch. The framework is very interesting.:) Also thanks for Evan's support.
Hi Ian,

Thanks for the review--I've fixed the error in my previous patch. (Hopefully we won't have to keep that custom form interaction code around for long!)

I wasn't sure if you wanted me to file a pull request on GitHub so that you could land it there, so I have not merged this patch.

You can find the pull request here: https://github.com/mozilla-b2g/gaia/pull/11699

Of course, I'd be happy to merge it myself if you prefer.
Attachment #792826 - Attachment is obsolete: true
Attachment #794158 - Flags: review?(iliu)
Comment on attachment 794158 [details] [diff] [review]
907177-add-integration-tests-v2.patch

Passed the integration test.

Note:
================================================================================
./bin/gaia-marionette ./apps/clock/test/marionette/clock_view_test.js 
./node_modules/.bin/mozilla-download --verbose --product b2g b2g
directory exists b2g exiting
npm WARN package.json marionette-client@0.13.3 No repository field.
npm WARN package.json marionette-js-runner@0.1.0 No repository field.
npm WARN package.json marionette-profile-builder@0.0.2 No repository field.

  ․․․

  3 passing (15 seconds)
================================================================================
Attachment #794158 - Flags: review?(iliu) → review+
(In reply to Mike Pennisi [:jugglinmike] from comment #4)
> Created attachment 794158 [details] [diff] [review]
> 907177-add-integration-tests-v2.patch
> 
> Hi Ian,
> 
> Thanks for the review--I've fixed the error in my previous patch. (Hopefully
> we won't have to keep that custom form interaction code around for long!)
> 
> I wasn't sure if you wanted me to file a pull request on GitHub so that you
> could land it there, so I have not merged this patch.
Oops! I would like to leave comment on github only. It's more convenient to click '+' for addressing comment than bugzilla.
> 
> You can find the pull request here:
> https://github.com/mozilla-b2g/gaia/pull/11699
> 
> Of course, I'd be happy to merge it myself if you prefer.
Sure, I also think merging pr by self is better.:)
master: https://github.com/mozilla-b2g/gaia/commit/55e465002133776a0700022b0bd9b629f6f7772d

Thanks for the review, Ian!
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I spoke with James, and he backed this commit out because it was causing intermittent failures on Travis CI:

https://travis-ci.org/mozilla-b2g/gaia/builds/10542297
https://travis-ci.org/mozilla-b2g/gaia/builds/10542463
https://travis-ci.org/mozilla-b2g/gaia/builds/10542667

Because the failures seem to be non-deterministic, we suspect this may be a bug in the Marionette client. I will report back on this bug as we learn more.
Blocks: 922128
We should also add some integration tests for the new features (Timer, Stopwatch, etc)
That's the idea.
I've overhauled the integration tests from the last patch and added tests to cover the new (for v1.2) panels: Stopwatch and Timer.

I've also submitted a pull request on GitHub:

https://github.com/mozilla-b2g/gaia/pull/12594

(In case it is helpful, you can find my individual commits at this closed pull request: https://github.com/mozilla-b2g/gaia/pull/11893)
Attachment #794158 - Attachment is obsolete: true
Attachment #812820 - Flags: feedback?(jlal)
Attachment #812820 - Flags: feedback?(bob.silverberg)
I've updated the pull request on GitHub to include the feedback from James and Bob.
Comment on attachment 812820 [details] [diff] [review]
907177-add-integration-tests-v3.patch

f+ from me... I have some small concerns about how we deal with display and the json file but I think the general concepts are great...

When it comes to refactoring the work done here out into a more focused lib we can talk about that again.
Attachment #812820 - Flags: feedback?(jlal) → feedback+
Attachment #812820 - Flags: feedback?(bob.silverberg) → feedback+
Hi Ian,

You might recognize this bug--we worked together on a patch that had to be backed out due to a race condition. I've worked closely with James Lal and Bob Silverberg to create a much more robust test suite that also covers the behavior of the Timer and Stopwatch panels.

This patch incorporates their feedback on see attachment 812820 [details] [diff] [review] and squashes all the commits into one. You can also find it on GitHub (along with all the discussion from the feedback process) at the following pull request:

https://github.com/mozilla-b2g/gaia/pull/12594

...and here's the passing TravisCI build report:

https://travis-ci.org/mozilla-b2g/gaia/builds/12339355

Thanks!
Attachment #815108 - Flags: review?(iliu)
Comment on attachment 815108 [details] [diff] [review]
907177-add-integration-tests-v4.patch

Review of attachment 815108 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Mike,

I felt embarrassed to reply the reviewing work late. I took some time to understand the code flow and module. This is very clear for me. And I learn some coding style in this patch that I never used. There are also some good idea in the patch. Besides, in part of alarm panel, if it's impossible, we could have a test for alarm goes off. I think it's more important for the main functionality. Not sure it's impossible or not in Marionette. In part of timer panel, it will test failed with reproduced rate 1/5. I guess there is a nit in the basic function of timer. Just try to click pause/resume button quickly. Then, you could see the incorrect countdown. I file Bug 927330 to track it. The script of integration test is right here. But we might fix Bug 927330 first. Then, we won't receive the failed report after land the patch here. Thanks for the awesome work:)
Attachment #815108 - Flags: review?(iliu) → review+
Hey Ian,

Of course I understand the delay--that was a lot of code to look over, after all. I appreciate your taking the time to review so thoroughly, and I'm glad you learned something in the process--I certainly did!

Good catch on Bug 927330--I did not experience these failures in my testing locally. Corey has already landed a fix for that bug, but I will mark it as "blocking" this bug just to be safe.
Depends on: 927330
master: https://github.com/mozilla-b2g/gaia/commit/cd0e6bba0a258ab9a9215d274bcc339d07ca9ab3

Many thanks to Bob, James, and Ian :)
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
We're experiencing a new deterministic failure in the test suite (for example: https://travis-ci.org/mozilla-b2g/gaia/jobs/12735545 ). This is unrelated to a change in Gaia, so I suspect it may be a bug in the platform (possibly related to bug 926587). Michael is helping me investigate.

backed out: https://github.com/mozilla-b2g/gaia/commit/c584b9ae7436c7cd134b568e17322fad0812f028
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The panel's enter animation ends before the panel is completely in the viewport. Michael identified this commit as introducing the regression:

http://hg.mozilla.org/mozilla-central/rev/45d9e6cd3473

and I have been able to reproduce the bug in Nightly. I am now working on a reduced test case to demonstrate the issue more directly. It may be somehow related to bug 921317, but it is still too early to say with any certainty.
https://hg.mozilla.org/mozilla-central/rev/f20d3f82fe2e
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
I am changing the status of this bug back to "Reopened" because, as reported in comment 20, I have backed the patch out of `master`.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The failure occurs due to the confluence of two bugs:

1. Because the integration test environment (b2g-desktop) does not implement pointer events, Marionette simulates them using mouse events. The Spinner does not prevent the default action of mouse events, so the simulated "flick" during the integration tests was carrying over into containing elements

2. A recent change in Gecko causes the Timer panel to expand well beyond its normal height. This caused the "carrying over" described by (1) to drastically affect the vertical position of the timer panel itself.

(1) will not be resolved until bug 822898 is resolved. I have filed bug 929757 against the Core::Layout module in order to address (2).

In the mean time, I will update this patch to add support for mouse events to the Spinner. This is a case of allowing the test implementation details "leak" into the application logic, which I normally wish to avoid. I believe it's permissible in this case because the change will be small (~3 lines of code) and the time frame for the necessary platform fixes is unclear.
Attached file Pull request on GitHub (obsolete) —
I believe I have addressed all the race conditions that led to intermittent failures in these tests. While I can't say with certainty, the last 84 consecutive runs on TravisCI have been successful [1]. This leads me to believe that the tests are stable enough for `master`.

[1] See the builds linked to from my previous pull request: https://github.com/mozilla-b2g/gaia/pull/13012
Attachment #8337893 - Flags: review?(jlal)
Comment on attachment 8337893 [details] [review]
Pull request on GitHub

Hi James,

I just remembered that we talked about splitting these tests across more files. I agree this is important, because as is, the files will tend to accumulate even more tests over time. I'll split them up, re-run the TravisCI build a few times, and request another review once I have a fresh patch.
Attachment #8337893 - Flags: review?(jlal)
Comment on attachment 8337893 [details] [review]
Pull request on GitHub

Sorry for the delay, James--splitting into multiple files seemed to trigger some strange timeout in the marionette-runner's `teardown` logic. I attempted to debug [1], but it looks like that has been cleared up.

[1] https://github.com/mozilla-b2g/gaia/pull/13012
Attachment #8337893 - Flags: review?(jlal)
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Sorry Mike, I had to revert this commit again. Seeing an intermittent failure, and trying to kill all those tonight so we can reopen the tree.

https://travis-ci.org/mozilla-b2g/gaia/jobs/15077387
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi James,

I don't know where to begin with this failure. I'm fairly confident that it has nothing to do with the Clock application, but I don't know how to interpret the error coming from:

    1) Alarm interaction "before each" hook:
    JavaScriptError: (17) NS_ERROR_UNEXPECTED: 
    Remote Stack:
    @app://system.gaiamobile.org/js/lockscreen.js, line 1092

Since it's a failure in a "before each" hook (although which before each--Marionette's or Clock's--is anybody's guess), my best guess is that the Lockscreen is interfering with navigation somehow. Do you agree? Do you think something like:

    marionette.client({ settings: { 'lockscreen.enabled': false } });

would help? If so, I guess we can't justify re-landing this patch until we've implemented this for *all* the Marionette tests (since they're all susceptible).
Flags: needinfo?(jlal)
FWIW, I don't think that error has anything to do with your tests either. When I backed this out, Gregor and I were simply trying to eliminate all intermittents so we could re-open the tree. But other tests were having the same problem, so this one was an unfortunate casualty.
Depends on: 961758
This error is not in the integration test framework. It is also not a failure in an integration-level concern (i.e. it is *not* caused by the lockscreen obscuring the Clock application during the tests). Instead, it is an error with the Lockscreen application itself. I have opened bug 961758 to track this error and marked this bug as a dependent. I'm also clearing the "needsinfo" request for James since this is application-specific.
Flags: needinfo?(jlal)
blocking-b2g: --- → backlog
Hi Michael,

I have been trying to trace the origin of this error through Gecko's source,
but I'm not very familiar with that component. Instead, I decided to approach
this from the top-down--I stepped through every failed Marionette JS test run
on Travis CI since the tests were removed.

There are more details in my comment on the Lockscreen bug [1], but it boils
down to this: the error continued to occur intermittently for 3 days and then
abruptly stopped.

Although this makes for an unsatisfying conclusion to my investigation, I
believe it is evidence enough to support re-introducing the tests. Would you
agree?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=961758#c3
Flags: needinfo?(mhenretty)
(In reply to Mike Pennisi [:jugglinmike] from comment #33)
> Although this makes for an unsatisfying conclusion to my investigation, I
> believe it is evidence enough to support re-introducing the tests. Would you
> agree?

Yup, I agree that the error is probably not related to your tests. Can we get a travis run with, say, 20 APP=clock integration tests. If they are all green on the first go, I think we can safely say these tests are ready to be put back. Here is a gist from kgrandon for doing something like that (just change TEST_INTEGRATION_APP_NAME to clock):

https://gist.github.com/KevinGrandon/8047042

Thanks for all your work on investigating this elusive problem!
Flags: needinfo?(mhenretty)
Thanks Michael. While I have already vetted these tests for race conditions (see comment 25), I am going to run them through the ringer once again in case any change to the application/platform has affected their behavior.
The platform changes introduced by bug 891882 have necessitated a change in the tests as they were merged on 2013.12.05 . This diff describes those changes.
Hi James,

The underlying issue that motivated the removal of these tests has since been resolved (see comment 33 and comment 34). Unfortunately, the platform has introduced an incompatible change since we last merged these tests, so they no longer pass as-is.

I'm requesting your review again because I've updated the patch to function correctly today--see attachment 8363918 [details] [diff] [review] for a description of the changes to the test files. (In addition, I removed the inclusion of the Marionette "forms" plugin because it has since been included by bug 959710.)

This pull request contains an additional commit (not intended for `master`) to trigger 21 consecutive integration test jobs on TravisCI; all 21 jobs completed successfully.

Thanks!
Attachment #8337893 - Attachment is obsolete: true
Attachment #8363924 - Flags: review?(jlal)
Whiteboard: [priority]
Here are 21 more consecutive successful test jobs: https://travis-ci.org/mozilla-b2g/gaia/builds/17477722
I'm not sure if James is still around/available to actually look at this... Gareth, any thoughts? I'd like to get this landed before Mike has to run off into the sunset.
Flags: needinfo?(gaye)
Mike - do you mind if I review instead? :)
Flags: needinfo?(gaye)
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

Gareth: I'd certainly appreciate that! This would have been easier for James, since he'd only be concerned with the diff I attached as attachment 8363918 [details] [diff] [review], but of course I'm happy to get feedback/approval from anyone with experience writing integration tests with the JavaScript Marionette client!

Marcus: Thanks for pushing on this--I'll be excited to continue with bug 922128 when it's landed. As for the sunset: I won't be running off into it all that soon :)
Attachment #8363924 - Flags: review?(jlal) → review?(gaye)
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

The test coverage here is AMAZING. I am so pleased with this!! I think we could massage things a little bit to make these tests easier to maintain, but in general this is so so so so so good! Thanks Mike!
Attachment #8363924 - Flags: review?(gaye)
Please feel free to flag me again if/when you clean things up a bit!
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

Hi Gareth,

Thanks for taking the time to carefully review all this code! I've incorporated most of your feedback, and left comments on the open-ended suggestions. Let me know how you want to move forward no those.

(The tests have passed another 21 times.)
Attachment #8363924 - Flags: review?(gaye)
Hi Mike!

I added a couple more comments. The biggest change that I would still like to see is breaking the large test blocks that test multiple behaviors out into separate tests.

I'm excited about getting this merged :).
Flags: needinfo?(mike)
Hi Gareth,

I incorporated your feedback and also updated the tests to pass lint. Thanks for the help!
Flags: needinfo?(mike)
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

Ok ship it!
Attachment #8363924 - Flags: review?(gaye) → review+
master: https://github.com/mozilla-b2g/gaia/commit/0ea959d81cefa0128157ec3ff0e2b7bdd29afacf

Thanks Gareth and James!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
I'm seeing this error when I run the marionette tests locally for stopwatch_lap_test.js... investigating.

~/moz/gaia $ ./bin/gaia-marionette apps/clock/test/marionette/stopwatch_lap_test.js                                                                                                           | 2:15PM Thu 6
./node_modules/.bin/mozilla-download  \
		--verbose \
		--product b2g \
		--channel tinderbox \
		--branch mozilla-central b2g
npm WARN package.json marionette-client@1.1.0 No repository field.
npm WARN package.json marionette-content-script@0.0.0 No repository field.
npm WARN package.json marionette-device-host@0.0.2 No repository field.
npm WARN package.json marionette-file-manager@0.0.2 No repository field.
npm WARN package.json marionette-profile-builder@0.0.3 No repository field.
npm WARN package.json mocha-json-proxy@0.0.7 No repository field.
Warning: Native modules not compiled.  XOR performance will be degraded.
Warning: Native modules not compiled.  UTF-8 validation disabled.
Warning: Native modules not compiled.  XOR performance will be degraded.
Warning: Native modules not compiled.  UTF-8 validation disabled.

  ․

  0 passing (3s)
  1 failing

  1) Lap creation lap advancement and ordering:

  AssertionError: Immediately-created lap entry contains nonzero time: expected "Lap 1
  00:00.00" to have a duration between 1ms and Infinityms
      at Function.assert.hasDuration (/Users/marcus/moz/gaia/apps/clock/test/marionette/lib/assert.js:112:5)
      at Context.<anonymous> (/Users/marcus/moz/gaia/apps/clock/test/marionette/stopwatch_lap_test.js:44:12)
      at Test.Runnable.run (/Users/marcus/moz/gaia/node_modules/mocha/lib/runnable.js:211:32)
      at Runner.runTest (/Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:372:10)
      at /Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:448:12
      at next (/Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:297:14)
      at /Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:307:7
      at next (/Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:245:23)
      at /Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:269:7
      at Hook.Runnable.run (/Users/marcus/moz/gaia/node_modules/mocha/lib/runnable.js:213:5)
      at next (/Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:257:10)
      at /Users/marcus/moz/gaia/node_modules/mocha/lib/runner.js:269:7
      at done (/Users/marcus/moz/gaia/node_modules/mocha/lib/runnable.js:185:5)
      at /Users/marcus/moz/gaia/node_modules/mocha/lib/runnable.js:197:9
      at Object.executeHook (/Users/marcus/moz/gaia/node_modules/marionette-client/lib/marionette/client.js:370:18)
      at process._tickCallback (node.js:415:13)
It seems like we need another wait between start() and lap() in that test, like so:

    stopwatch.start();
+   client.helper.wait(11);

    stopwatch.lap();

otherwise the first lap will be 0? I'm surprised it's passing on Travis.
Flags: needinfo?(mike)
reverted: https://github.com/mozilla-b2g/gaia/commit/ba34d21c5f3d71030ef0893b2341f16a03a8e40c

You're right: there is a race condition here. It's exposed when the `stop` and `lap` operations complete within less than 1 millisecond of each other, but the TravisCI environment is not capable enough to trigger this condition (the same goes for my system). Of course, if your machine can trigger the race, the patch isn't suitable for `master`. I've backed it out.
Status: RESOLVED → REOPENED
Flags: needinfo?(mike)
Resolution: FIXED → ---
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

Hey Marcus,

I've updated this pull request to include the change you requested. Could you verify that it correctly the race condition you identified on your machine?

Thanks!
Attachment #8363924 - Flags: review+ → review?(m)
Comment on attachment 8363924 [details] [review]
Pull request on GitHub.com

Yes, that fixes it on my machine.
Attachment #8363924 - Flags: review?(m) → review+
master: https://github.com/mozilla-b2g/gaia/commit/f1d54ea83178ce64c5ab040fd6bedd7700cd8325

Thanks, Marcus!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.