Closed Bug 1171577 Opened 5 years ago Closed 5 years ago

LogCapture Gonk-specific tests fail on Flame device

Categories

(Firefox OS Graveyard :: Developer Tools, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
FxOS-S1 (26Jun)
Tracking Status
firefox41 --- fixed

People

(Reporter: hobinjk, Assigned: hobinjk)

Details

(Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150529130603

Steps to reproduce:

Ran xpcshell tests of LogCapture using a flame device on the latest base image.


Actual results:

test_logcapture_gonk.js failed an assertion because the ro.kernel.qemu property was undefined.


Expected results:

test_logcapture_gonk should not have depended on a property that reflects whether the device happens to be running hardware virtualization (it likely only exists on the emulator)
Whiteboard: [systemsfe]
Attached patch bug1171577-logcapture-gonk.patch (obsolete) — Splinter Review
This patch fixes the test failures locally, but should be verified on emulators and the automated test platform. I'm worried that init.svc.b2g can be "restarted" in addition to "running"
Attachment #8615452 - Flags: review?(lissyx+mozillians)
Comment on attachment 8615452 [details] [diff] [review]
bug1171577-logcapture-gonk.patch

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

I agree that it's tightly coupled and decoupling is probably a good thing since we will want more device tests. However, why this specific property then?
Given I share your concern, we should find a property that is both available on emulator AND device, and NOT changing.
I think I have never saw ro.product.locale.language and ro.product.locale.region changing, and they should be on both.

What about checking them then?
> ro.product.locale.language=en
> ro.product.locale.region=US
Attachment #8615452 - Flags: review?(lissyx+mozillians)
Aren't builds tested using alternate locales? That sounds like a property that could change very frequently.

A test that might be good to add either as a supplement or a replacement is to verify that all of the strings read as properties are ASCII format and of expected length. system_properties.h defines the maximum allowed lengths.
(In reply to James Hobin from comment #3)
> Aren't builds tested using alternate locales? That sounds like a property
> that could change very frequently.

Yes but we don't use this :)

> 
> A test that might be good to add either as a supplement or a replacement is
> to verify that all of the strings read as properties are ASCII format and of
> expected length. system_properties.h defines the maximum allowed lengths.

I'm okay with this.
Assignee: nobody → hobinjk
Attached patch bug1171577.patchSplinter Review
This incorporates the feedback in comment 2 and adds the additional condition that all property values must be strings.
Attachment #8615452 - Attachment is obsolete: true
Attachment #8621889 - Flags: review?(lissyx+mozillians)
Comment on attachment 8621889 [details] [diff] [review]
bug1171577.patch

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

::: b2g/components/test/unit/test_logcapture_gonk.js
@@ +42,5 @@
> +  equal(propertiesLog["ro.product.locale.language"], "en",
> +        "Locale language should be read correctly. See bug 1171577.");
> +
> +  equal(propertiesLog["ro.product.locale.region"], "US",
> +        "Locale region should be read correctly. See bug 1171577.");

Good but maybe we could simplify this:

const kExpectedProperties = [ {
  name:  "ro.product.locale.language",
  type:  "string",
  value: "en"
}, {
  name:  "ro.product.locale.region",
  type:  "string",
  value: "US"
} ];

kExpectedProperties.forEach(e => {
  notEqual(propertiesLog.indexOf(e.name), -1, "should have property");
  equal(typeof(propertiesLog[e.name]), e.type, "property type mismatch");
  equal(typeof(propertiesLog[e.name]), e.value, "property value mismatch");
});
Attachment #8621889 - Flags: review?(lissyx+mozillians) → review+
That feels like a decrease in readability of the test. Additionally, propertiesLog.indexOf wouldn't work.

While having a generic checking function is a good idea, I think that future modifications to this code are not going to be changing the expected properties. Instead, they will probably scrap this test entirely in favor of fully overwriting and controlling the properties system.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d43d971b4b9e
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S1 (26Jun)
You need to log in before you can comment on or make changes to this bug.