982797 - Robocop: Switch tests from using waitForTest to waitForCondition

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Chenxia Liu [:liuche]]

Robotium 4.2 added Conditions, and we should replace all instances of waitForTest with waitForCondition.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Reset assignee due to inactivity.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

And a list of all instances of `waitforTest(`: https://mxr.mozilla.org/mozilla-central/search?string=waitForTest%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 3

[David Lubin]

Hi. I'd like to confirm that the only change you'd like performed is to change the method from waitForTest to waitForCondition, right? Are the parameters the same? If so, I believe I can have a patch submitted shortly.

Comment 4

[David Lubin]

Attachment: waitForCondition.patch

I replaced all instances of waitForTest (at https://mxr.mozilla.org/mozilla-central/search?string=waitForTest%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central) with waitForCondition. Please let me know if this is sufficient.

Comment 5

[Chenxia Liu [:liuche]]

Comment on attachment 8516110 [details] [diff] [review]
waitForCondition.patch

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

Hi David! Thanks for picking up this bug!

Taking a quick glance, I see a few big changes you should make to this patch before it's ready for review.

Take a look at the waitForCondition method in BaseTest.java - it's right before BooleanTest and waitForTest. The signature is not in fact the same as waitForTest, so you'll need to change that as well.

If you want to do a partial build with this patch to catch compile errors, you can just build the tests with:
mach build $TOP_SRC_DIR/build/mobile/robocop

where TOP_SRC_DIR is your code directory.

Clearing review flag for these changes - this needs a little more work.

::: mobile/android/base/tests/BaseTest.java
@@ +330,5 @@
>      }
>  
>      // TODO: With Robotium 4.2, we should use Condition and waitForCondition instead.
>      // Future boolean tests should not use this method.
> +    protected final boolean waitForCondition(BooleanTest t, int timeout) {

You should remove this instance of waitForTest completely, because this bug is for switching calls of the deprecated waitForTest to the new waitForConditon call.

In fact, if waitForTest is the only consumer of BooleanTest, you can remove that too! :)

Comment 6

[David Lubin]

I see that this is more complicated than I thought. So it looks like, as you said, the method that switches calls of waitForTest to waitForCondition and BooleanTest can be deleted. Since waitForCondition uses conditions instead of booleantests, this means that the parameters for the new method calls needs to be different. Could you please advise on how to handle situations in which the condition parameter is given an overridden method, such as in this example:

boolean correctText = waitForTest(new BooleanTest() {
            @Override
            public boolean test() {
                final String clipboardText = Clipboard.getText();
                mAsserter.dumpLog("Clipboard text = " + clipboardText + " , expected text = " + copiedText);
                return clipboardText.contains(copiedText);
            }
        }, MAX_TEST_TIMEOUT);

 I'm having trouble finding the declaration of Condition. Can I just leave the "public boolean test()" as it is, or does it need to be replaced with something?

Comment 7

[David Lubin]

I'd like to add on to the last question, would it be enough to replace "public boolean test()" with "public boolean isSatisfied()" and leave the definition unchanged?

Comment 8

[Chenxia Liu [:liuche]]

Condition is part of the Robotium framework, so it wouldn't actually be in our code base. Here's the link to the Robotium API:

http://robotium.googlecode.com/svn/doc/index.html

(this is not exactly the version that we're using, but the API is the same)

So yes, you are correct! You can just switch the test() method to isSatisfied() when you replace waitForTest().

Comment 9

[David Lubin]

Attachment: waitForConditionV2.patch

I believe this patch handles the changes discussed. Please let me know what you think.

Comment 10

[Chenxia Liu [:liuche]]

Comment on attachment 8516373 [details] [diff] [review]
waitForConditionV2.patch

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

Hi David, thanks for the patch. You're pretty close! A few more things:

I would recommend applying your patches to your local copy of the tree and at least building successfully before requesting review. For the files that now use Condition instead of BooleanTest, you'll also need to import Condition so that the patch will build. (As a side note, you should pull your tree again because there's a little bitrot from bug 1092254 having landed, so the patch isn't applying cleanly for me.)

If you have already built Fennec, you can do a partial build by running:
mach build build/mobile/robocop
from the top directory of the source tree. This should only take a minute!

If you haven't built Fennec, take a look at https://wiki.mozilla.org/Mobile/Fennec/Android#Building_and_deploying_the_code . The rest of that wiki is also a good place to make sure you have everything set up.

If you need help, feel free to jump into #mobile and ask! I and other members of the mobile team are around during PST work hours, and are happy to help.

Comment 11

[David Lubin]

Attachment: waitForConditionV3.patch

Thanks for your help so far. I pulled the latest changes and fixed the patch so that it would apply properly. This latest patch includes Condition imports for the files that did not previously have it, but I'm not sure whether or not build is currently working, so I just flagged for feedback. When building, both with and without the patch, I'm receiving a message saying "For some reason, Clobber had problems applying the changes for Android in Bug 1091118," and it seems to be failing. Do you have any idea why I might be having this issue, even without any patches on my Mercurial queue?

Comment 12

[Chenxia Liu [:liuche]]

Hi David, I'm not sure what this problem is - can you drop the relevant part of your log into pastebin?

http://pastebin.mozilla.org/

Also, make sure you've got all the build dependencies from https://wiki.mozilla.org/Mobile/Fennec/Android .

Again, I encourage you to drop into #mobile on Mozilla IRC (connect to irc.mozilla.org) - it will definitely be easier to debug this problem, and there are more people there who might have run into this problem.

Comment 13

[Chenxia Liu [:liuche]]

Comment on attachment 8516410 [details] [diff] [review]
waitForConditionV3.patch

Clearing the feedback flag until we figure out the building problems.

In general, before uploading a patch, you should always make sure it is building and try to verify that the changes that you have made are working as expected. Otherwise, it doesn't really make sense for someone else to review or give feedback on the patch because it'll have to change anyways.

Take a look at the instructions at https://wiki.mozilla.org/Mobile/Fennec/Android for setting up a build again, and make sure you've done all the steps - this should get you to a working build of the tree.

Comment 14

[Andy Pusch [:AndyP]]

I'd like to work on this bug, could someone assign me please?

Comment 15

[Richard Newman [:rnewman]]

Andy: just get started! We can mark it as assigned when you've got a patch underway.

Comment 16

[Andy Pusch [:AndyP]]

Attachment: bug982797_waitForTestReplacement.diff

I've successfully built, installed and run Fennec with these changes. No further testing done.

Comment 17

[Andy Pusch [:AndyP]]

I forgot to mark the old patch as obsolete, sorry. Is there a way to change that?

The problem with the old patch was possibly because David forgot the two methods in testFindInPage.java. Probably because the link in comment #2 did not include the file because the waitForTest methods there had a space before the "(".

Comment 18

[Chenxia Liu [:liuche]]

Thanks for the patch, Andy! Since you're fixing a test file, building and running Fennec won't reflect the changes you've made, unless you run the tests locally.

We have instructions for running tests locally, but sometimes they can be flaky :/
https://wiki.mozilla.org/Mobile/Fennec/Android#Robocop

In the meantime, I've pushed to our Try server for you (which will run our tests), so we'll see how those tests run.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a421d559d66

If you end up working on more tests, we can definitely give you push access to the try server. :)

You can mark an attachment "obsolete" by clicking on the "Details" link, and then clicking "edit details" next to the patch name; there is a checkbox for "obsolete". (You can also un-obsolete patches that way.)

Thanks for your work, Andy!

Comment 19

[Chenxia Liu [:liuche]]

I forgot to mention: if you want to try compiling the tests, you can run |mach build build/mobile/robocop|, which builds our robocop tests. Good news, your changes to the tests do in fact compile :)

Comment 20

[Chenxia Liu [:liuche]]

Comment on attachment 8562731 [details] [diff] [review]
bug982797_waitForTestReplacement.diff

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

Great job, just a couple of nits! Once you change that, upload a new patch, obsolete the old ones, and I'll r+ the new patch (or you can move the r+ to that patch).

The commit message should also match the name of the bug and include the reviewer, so for this bug:

Bug 982797 - Robocop: Switch tests from using waitForTest to waitForCondition. r=liuche

::: mobile/android/base/tests/testFindInPage.java
@@ +20,2 @@
>  public class testFindInPage extends JavascriptTest implements GeckoEventListener {
>      private static final int WAIT_FOR_TEST = 3000;

Nit: Let's rename this so it matches the methods better, and also specify the unit of time. WAIT_FOR_CONDITION_MS

@@ +98,5 @@
>                          return false;
>                      }
>                  }
>              }, WAIT_FOR_TEST);
>              mSolo.sleep(500); // TODO: Find a better way to wait here because waitForTest is not enough

One nit - this references waitForTest, which is now no longer in the code base. Will you update this to refer to waitForCondition?

Comment 21

[Andy Pusch [:AndyP]]

Attachment: bug982797_waitForTestReplacement.diff - v2

I've made the changes as requested. For some reason I cannot mark the other patch obsolete though.

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8564223 [details] [diff] [review]
bug982797_waitForTestReplacement.diff - v2

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

Nice! Thanks Andy :)

Comment 23

[Chenxia Liu [:liuche]]

Green try run from a few days ago, only comment and var changes since then:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a421d559d66

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca1304f0b42

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4ca1304f0b42