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

NEW
Unassigned

Status

()

Firefox for Android
Testing
P3
normal
a year ago
9 months ago

People

(Reporter: ahunt, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 affected)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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 hidden (mozreview-request)
(Reporter)

Updated

a year ago
Blocks: 1330454
(Reporter)

Updated

a year ago
Attachment #8838715 - Flags: review?(gkruglov)
Iteration: 1.15 → 1.16
(Reporter)

Updated

a year ago
Iteration: 1.16 → ---
Priority: P1 → P2

Comment 2

a year ago
mozreview-review
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.
Attachment #8838715 - Flags: review?(gkruglov) → review+

Comment 3

a year ago
mozreview-review
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`.
Blocks: 1377259

Updated

9 months ago
Assignee: andrzej → nobody

Updated

9 months ago
Priority: P2 → P3
You need to log in before you can comment on or make changes to this bug.