Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a unit test for extract area out of bounds #498

Merged
merged 3 commits into from
Jul 8, 2016
Merged

Add a unit test for extract area out of bounds #498

merged 3 commits into from
Jul 8, 2016

Conversation

mhirsch
Copy link
Contributor

@mhirsch mhirsch commented Jul 7, 2016

This unit test checks to see that an assertion is thrown if extract is called for an area outside the image size.

This test doesn't pass right now. I'm not sure if this is even a good test to have -- but I'm submitting a pr in order to understand better. What's the recommended way to throw a proper exception from within native code in node? Obviously it isn't possible to know at the time execute is called in the js layer whether the area falls within the image bounds. So this will be caught in pipeline.cc, Execute(). There's this (baton->err).append(...); return Error(); pattern, but that results in the same type of error handling as an error that is thrown from vips, e.g.:

Unhandled rejection Error: extract_area: bad extract area

    at Error (native)

The test suite doesn't see that as a valid exception, so the unit test fails.

@lovell
Copy link
Owner

lovell commented Jul 7, 2016

The test needs to include toBuffer( ... ) to invoke the pipeline.

@mhirsch
Copy link
Contributor Author

mhirsch commented Jul 7, 2016

With this test, there is an Unhandled rejection Error thrown -- you can see this on the node command line, but that doesn't register as the assert that the test suite is looking for. Are you aware if there's a way to catch this type of error and throw a proper assert?

@lovell
Copy link
Owner

lovell commented Jul 8, 2016

Once you're in C++ land, any error becomes the first parameter of the invoked callback function, e.g.:

it('Bad image area', function(done) {
  sharp(fixtures.inputJpg)
    .extract({ left: 3000, top: 10, width: 10, height: 10 })
    .toBuffer(function(err) {
      assert(err instanceof Error);
      done();
    });
});

@mhirsch
Copy link
Contributor Author

mhirsch commented Jul 8, 2016

Ok! That's what I was looking for. Thanks.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage remained the same at 98.371% when pulling c4449dd on mhirsch:extract_bounds_unit_test into 8dd554b on lovell:master.

@lovell lovell merged commit 673d827 into lovell:master Jul 8, 2016
@lovell
Copy link
Owner

lovell commented Jul 8, 2016

Thank you!

@mhirsch mhirsch deleted the extract_bounds_unit_test branch July 8, 2016 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants