Closed Bug 976802 Opened 6 years ago Closed 6 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mikeh, Assigned: mikeh)

Details

Attachments

(1 file, 2 obsolete files)

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.
Summary: [Camera][Test] More automated tests → [Camera][Test] Fake CameraParameter injection for automated testing
Updated version of the patch that successfully tests the zoom parameter. Work in progress: need to add more tests.
Attachment #8381756 - Attachment is obsolete: true
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?
Attachment #8383438 - Attachment is obsolete: true
Attachment #8383767 - Flags: review?(dhylands)
Attachment #8383767 - Flags: review?(dflanagan)
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.
Attachment #8383767 - Flags: review?(dflanagan)
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.
Attachment #8383767 - Flags: review?(dhylands) → review+
(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.
(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()
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.
https://hg.mozilla.org/mozilla-central/rev/28d7d39701f5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.