976802 - [Camera][Test] Fake CameraParameter injection for automated testing

Status: RESOLVED FIXED

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

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

Because we run on the emulator, so we're somewhat limited in terms of the capabilities we can test. For example, the emulator does not support zooming and doesn't have a flash.

I want to make use of the test shim I added in bug 940424 to add the ability to inject fake capabilities and settings into the camera stack so that parsing/handling code can be tested.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - add ability to inject fake CameraParameters, v1

Comment 2

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - add ability to inject fake CameraParameters, v2

Updated version of the patch that successfully tests the zoom parameter. Work in progress: need to add more tests.

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=b1ff9b24f584

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Add ability to inject fake CameraParameters, v3

Currently this includes a test that fakes zoom support in the emulator. Other fake tests to follow, but for now I want to land this so that other pending patches can make use of it.

dhylands, can you review the C++ stuff?
djf, would you mind reviewing the HTML/JS stuff?

Comment 5

[David Flanagan [:djf]]

Comment on attachment 8383767 [details] [diff] [review]
Add ability to inject fake CameraParameters, v3

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

Clearing the review request since I don't know the first thing about writing tests in gecko.  I didn't see anything obviously wrong with the JS code, but I didn't understand the JS code.  I suspect that Dave will find it much easier to review the HTML and JS parts than I would.

::: dom/camera/test/test_camera_fake_parameters.html
@@ +126,5 @@
> +      }
> +    }
> +  };
> +  next();
> +});

I don't understand this part. I don't see CameraTest defined anywhere. I'm not sure why you're calling begin() on it, but are defining the next() and run() functions on it.

But the real thing that I don't get is why run() is defined inside of next() and next() is defined inside of the begin callback. If you can flatten things out it might be easier to understand.  But I basically know nothing at all about writing gecko tests, so it may just be that this is all over my head.

Comment 6

[Dave Hylands [:dhylands]]

Comment on attachment 8383767 [details] [diff] [review]
Add ability to inject fake CameraParameters, v3

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

Looks reasonable to me.

::: dom/camera/GonkCameraParameters.cpp
@@ +636,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +
> +  aArray.Clear();

Should this be up a few lines so that it gets cleared if we take the return on line 637?

::: dom/camera/test/test_camera_fake_parameters.html
@@ +127,5 @@
> +    }
> +  };
> +  next();
> +});
> +

CameraTest is defined in camera_common.js (referenced on line 7)

I make no claims to understand the test stuff either, but I figure as long as it runs, and I don't see anything obviously wrong, then thats good enough.

The only thing I see is that because each test calls next(), you wind up with a huge callstack. So if the next can be taken out and made part of the framework, then it doesn't stack up.

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Dave Hylands [:dhylands] from comment #6)
> Comment on attachment 8383767 [details] [diff] [review]
> Add ability to inject fake CameraParameters, v3
> 
> Review of attachment 8383767 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me.
> 
> ::: dom/camera/GonkCameraParameters.cpp
> @@ +636,5 @@
> >    if (NS_FAILED(rv)) {
> >      return rv;
> >    }
> > +
> > +  aArray.Clear();
> 
> Should this be up a few lines so that it gets cleared if we take the return
> on line 637?

In the case of the return on line 637, the error code will be set, so I think it makes sense to leave the array as-is.

> ::: dom/camera/test/test_camera_fake_parameters.html
> @@ +127,5 @@
> > +    }
> > +  };
> > +  next();
> > +});
> > +
> 
> CameraTest is defined in camera_common.js (referenced on line 7)
> 
> I make no claims to understand the test stuff either, but I figure as long
> as it runs, and I don't see anything obviously wrong, then that's good enough.
> 
> The only thing I see is that because each test calls next(), you wind up
> with a huge callstack. So if the next can be taken out and made part of the
> framework, then it doesn't stack up.

In previous tests where I've structured the code like this, there were always async CameraControl methods to break up the callstack; but not for this test. I'll look into that.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Mike Habicher [:mikeh] from comment #7)
>
> In previous tests where I've structured the code like this, there were
> always async CameraControl methods to break up the callstack; but not for
> this test. I'll look into that.

Actually, the call to getCamera() breaks up the callstack. It never gets (approximately) deeper than:
  onSuccess()
    t.test()
      next()
        t.prep()
          t.run()

Comment 9

[Mike Habicher [:mikeh] (high bugzilla latency)]

Oh, and there's a callback hidden in the setFakeParameters() call (in t.prep()) too that also breaks up the callstack. So I don't think this is an issue.

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

https://hg.mozilla.org/integration/b2g-inbound/rev/28d7d39701f5

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/28d7d39701f5