1340662 - Allow running code off the main thread in roboelectric / junit 4

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Testing, defect, P5)

Description

[Andrzej Hunt :ahunt]

I'd like to test some new DomainProcessor cod (Bug 1330454), that code has to be run off the main thread and therefore uses ThreadUtils.assertNotOnUiThread(). RoboElectric runs test on the main thread, so that assert fails.

Using an assert to ensure code isn't being run on the UI thread seems to be uncommon, i.e. not a scenario that RoboElectric supports, so we need to add our own alternative.

One possiblity is simply disabling the assert when running tests - that requires cluttering up the app code though. We can roll our own test-side solution to this.

Comment 1

[Andrzej Hunt :ahunt]

Attachment: Bug 1340662 - Implement utility to run Roboelectric test code on a background thread

Review commit: https://reviewboard.mozilla.org/r/113518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113518/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8838715 [details]
Bug 1340662 - Implement utility to run Roboelectric test code on a background thread

https://reviewboard.mozilla.org/r/113518/#review117952

r+, but i think you might have a threading issue.

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/TestRunner.java:154
(Diff revision 1)
> +        // Any exceptions that are thrown on a Thread will be silently ignored by default. We need to store
> +        // the exception and rethrow it ourselves later. The Thread will terminate once an exception
> +        // is thrown, so we don't need a list here - however we can't set references to new objects
> +        // on different threads (i.e. we need a final reference here), and Optional<> isn't available
> +        // until API 24 - so we just use a list as the simplest equivalent.
> +        final LinkedList<Throwable> throwables = new LinkedList<>();

You're reading/writing to this list from different threads, but not concurrently. iiuc, your `if` check below is not guaranteed to see added throwables.

Consider using something like ConcurrentLinkedQueue as a `volatilie` alternative for the collection members to ensure visibility of changes across threads.

Comment 3

[:Grisha Kruglov]

Comment on attachment 8838715 [details]
Bug 1340662 - Implement utility to run Roboelectric test code on a background thread

https://reviewboard.mozilla.org/r/113518/#review117958

::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/background/testhelpers/TestRunner.java:166
(Diff revision 1)
> +        });
> +
> +        thread.start();
> +        thread.join();
> +
> +        if (!throwables.isEmpty()) {

If you do use a queue, you'll be able to avoid this non-atomic construct and instead do `pop` and check for `null`.

Comment 4

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 5

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.