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)

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"

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

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.

This incorporates the feedback in comment 2 and adds the additional condition that all property values must be strings.

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"); });

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.

try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=230014280227