Closed Bug 987825 Opened 7 years ago Closed 7 years ago

build tests are failing because we've added camera configuration options

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

x86
macOS
defect
Not set
normal

Tracking

(b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
Tracking Status
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: djf, Assigned: djf)

Details

Attachments

(1 file)

As part of the camera refactoring, we've changed the camera build system to be independent of the build/ directory, and we've added new configuration options, so our generated config files are no longer the same as they were when the build was being handled by the build/ scripts.

Build tests are failing now, and I think we should just disable those tests and remove any camera-specific build steps from the build/ directory if they are no longer needed.
Assignee: nobody → dflanagan
This patch just removes the camera build tests that were failing.

They were much too brittle, especially given that we will be changing our build time configuration system fairly dramatically.
Attachment #8396538 - Flags: review?(dmarcos)
Comment on attachment 8396538 [details] [review]
link to github pull request

Changing review request to Gareth.

Yuren: I've set feedback? so you're aware of this change.
Attachment #8396538 - Flags: review?(gaye)
Attachment #8396538 - Flags: review?(dmarcos)
Attachment #8396538 - Flags: feedback?(yurenju.mozilla)
Comment on attachment 8396538 [details] [review]
link to github pull request

It looks like this patch already landed, but I still have comments :).

The tests that were disabled checked to make sure that gaia make

1) generates a js camera configuration file
2) that it gets put in the right place, and (very inflexibly)
3) that it exactly (string) matches what we expect

I agree whole-heartedly that the tests were written incorrectly. However, I think simply deleting them is not the best thing here. Deleting tests is ok if the product/code are changing so that we no longer expect the behavior we're verifying in the tests. However, I gather (since the patch lacked non-test code) that we still expect for make to generate camera configuration code. We have now left the build system vulnerable to evil coders trying to break things without anyone noticing :).

In this case I would prefer if we replace the badly devised tests with better ones. I think a better integration-level test would run the config file code generated by make (perhaps in a sandbox like http://nodejs.org/api/vm.html) and check to make sure that it set the correct configuration in the javascript context, made the right function calls, etc.

Thoughts?
Attachment #8396538 - Flags: review?(gaye)
The patch has landed only on camera-new-features so that we can get a Travis build running to see if it fixes the problems we are having. It has not been uplifted to master yet.

I agree, in principle, that there ought to be tests of the camera build system. We have pending work that will completely change our build-time configuration stuff, so working to update the brittle test now doesn't make sense.

These build tests were introduced without any input from the media team and they have broken our workflow, and we really have to remove them today in order to get 1.4+ work done.
Comment on attachment 8396538 [details] [review]
link to github pull request

got it, thanks.
Attachment #8396538 - Flags: feedback?(yurenju.mozilla) → feedback+
Landed: https://github.com/mozilla-b2g/gaia/commit/3042c01a9914332a18f64faee2e71e32f3b45dd3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Bulk edit for camera bugs.

If earlier comments do not show how this bug landed to master, it probably landed as part of https://github.com/mozilla-b2g/gaia/pull/17599 which merged the camera-new-features branch into master.

This bug was uplifted from master to v1.4 as part of https://github.com/mozilla-b2g/gaia/commit/a8190d08e61316a86bba572ba8d894d081a20530
Target Milestone: --- → 1.4 S5 (11apr)
You need to log in before you can comment on or make changes to this bug.