third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html - Issue 2441953002: Shape Detection: Add two layout tests for face detection

Side by Side Diff: third_party/WebKit/LayoutTests/fast/shapedetection/shapedetection-security-test.html

Issue 2441953002: Shape Detection: Add two layout tests for face detection (Closed)

Patch Set: Created 4 years, 2 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « no previous file | third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp » ('j') | third_party/WebKit/Source/modules/shapedetection/FaceDetector.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>

	2 <script src=../../resources/testharness.js></script>

	3 <script src=../../resources/testharnessreport.js></script>

	4 <script>

	5

	6 // TODO(xianglu): Consider tests for image with only one dimension equal to zero .
	mcasas 2016/10/21 04:41:45 "Consider adding ..." Although there's no hard li "Consider adding ..." Although there's no hard limit of 80 characters in JS files that I know of, it's a good idea to stick to it, especially since here you have two lines xianglu 2016/10/21 21:08:30 Done. Show quoted text On 2016/10/21 04:41:45, mcasas wrote: > "Consider adding ..." > > Although there's no hard limit of 80 characters in JS files > that I know of, it's a good idea to stick to it, especially since > here you have two lines Done.
	7

	8 // This test verifies that FaceDetector will reject an empty image.

	9 async_test(function(rejectEmptyImageTest){

	10 var emptyImage = new Image();

	11 rejectEmptyImageTest.step(function() {

	12 assert_equals(emptyImage.naturalWidth + emptyImage.naturalHeight, 0);
	mcasas 2016/10/21 04:41:46 Here you can just assert_equals(), no need to crea Here you can just assert_equals(), no need to create and added test step such as l.11. Same elsewhere: since we're inside an async_test(), you can directly assert_(), no need for extra step()s. xianglu* 2016/10/21 21:08:30 The difference is that without a step function, th Show quoted text On 2016/10/21 04:41:46, mcasas wrote: > Here you can just assert_equals(), no need to create > and added test step such as l.11. Same elsewhere: > since we're inside an async_test(), you can directly > assert_*(), no need for extra step()s. The difference is that without a step function, the test result will be TIMEOUT when an assert fails. I might prefer to see a FAIL in that case.
	13 });

	14 //No need to call onload() because an empty image never fires this event.

	15 var faceDetector = new FaceDetector();

	16 faceDetector.detect(emptyImage)

	17 .then(faceDetectionResult => {
	mcasas 2016/10/21 04:41:45 Usually .then() and .catch() clauses are indented Usually .then() and .catch() clauses are indented 4 spaces to the right. xianglu 2016/10/21 21:08:30 Done. Show quoted text On 2016/10/21 04:41:45, mcasas wrote: > Usually .then() and .catch() clauses are indented 4 > spaces to the right. Done.
	18 rejectEmptyImageTest.step(function() {

	19 assert_unreached("Promise for empty images should not be resolved.");

	20 });

	21 rejectEmptyImageTest.done();

	22 })

	23 .catch(error => {

	24 rejectEmptyImageTest.step(function() {

	25 assert_equals(error.name, "InvalidStateError");

	26 assert_equals(error.message, "HTMLImageElement is empty.");

	27 });

	28 rejectEmptyImageTest.done();

	29 });
	mcasas 2016/10/21 04:41:45 This whole l.16-29 should be simplified to: asser This whole l.16-29 should be simplified to: assert_throws("InvalidStateError", function() { faceDetector.detect(emptyImage); }, "FaceDetector throws InvalidStateError when called with an empty Image"); Same for l.37-50, it should simplify this code quite a bit. xianglu 2016/10/21 21:08:30 I think instead throwing an exception itself, face Show quoted text On 2016/10/21 04:41:45, mcasas wrote: > This whole l.16-29 should be simplified to: > > assert_throws("InvalidStateError", > function() { > faceDetector.detect(emptyImage); > }, > "FaceDetector throws InvalidStateError when called with an empty > Image"); > > Same for l.37-50, it should simplify this code > quite a bit. I think instead throwing an exception itself, faceDetector.detect(emptyImage) actually returns a promise which throws an exception. In other words, the exception is not thrown by detect().
	30 },"FaceDetector should reject empty images with InvalidStateError.");

	31

	32 // This test verifies that FaceDetector will reject an undecodable image.

	33 async_test(function(rejectUndecodableImageTest){

	34 var undecodableImage = new Image();

	35 undecodableImage.onload = function () {

	36 var faceDetector = new FaceDetector();

	37 faceDetector.detect(undecodableImage)

	38 .then(faceDetectionResult => {

	39 rejectUndecodableImageTest.step(function() {

	40 assert_unreached("Promise for undecodable images should not be resolved. ");

	41 });

	42 rejectUndecodableImageTest.done();

	43 })

	44 .catch(error => {

	45 rejectUndecodableImageTest.step(function() {

	46 assert_equals(error.name, "InvalidStateError");

	47 assert_equals(error.message, "HTMLImageElement is broken.");

	48 });

	49 rejectUndecodableImageTest.done();

	50 });

	51 };

	52 undecodableImage.src = "../../imported/wpt/images/broken.png";

	53 }, "FaceDetector should reject undecodable images with InvalidStateError.");

	54

	55

	56 </script>

OLD	NEW