Add unit-tests for colorspace.js #8611

apoorv-mishra · 2017-07-05T11:33:43Z

Hi, I have added some tests to check the isDefaultDecode(...) function in colorspace.js. I'll be adding the tests for other functions too. Please review and let me know the necessary changes to be made.

Thanks.

Fixes #7261.

Snuffleupagus · 2017-07-05T12:01:36Z

test/unit/colorspace_spec.js

+  it('should be true if decode is not an array', function() {
+    let test1 = new Test('I am a string', 0);
+    let result1 = true;
+    let returnValue1 = ColorSpace.isDefaultDecode(test1.decode, test1.n);


Considering the tests that you've currently added in the patch, I'm not sure why the code needs to be so verbose in these test-cases!?
Something along these lines ought to be perfectly sufficient:

expect(ColorSpace.isDefaultDecode('string', 0)).toBeTruthy(); // or .toBeFalsy() where applicable

or, if that doesn't fit on one line (e.g. below when you're passing arrays as arguments):

let someVariableName = [...]; expect(ColorSpace.isDefaultDecode(someVariableName, 0)).toBeTruthy();

Snuffleupagus · 2017-07-05T12:03:50Z

test/unit/colorspace_spec.js

+import { ColorSpace } from '../../src/core/colorspace';
+
+describe('colorspace', function () {
+  class Test {


I don't think that this is necessary; see the suggestion in https://github.com/mozilla/pdf.js/pull/8611/files#r125623641.

Wait!, don't you need to import ColorSpace?

Snuffleupagus · 2017-07-05T12:05:49Z

test/unit/colorspace_spec.js

+    let result5 = false;
+    let returnValue5 = ColorSpace.isDefaultDecode(test5.decode, test5.n);
+
+    let test6 = new Test([0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 1, 0, 0, 1], 8);


In practice, I don't think the number of components will usually go beyond 4, so you can probably use slightly shorter arrays here (and below too).

apoorv-mishra · 2017-07-05T12:54:28Z

Hi @Snuffleupagus , I made the appropriate changes. Please have a look and let me know.

Snuffleupagus · 2017-07-05T12:59:05Z

test/unit/colorspace_spec.js

+
+describe('colorspace', function () {
+  it('should be true if decode is not an array', function() {
+    expect(ColorSpace.isDefaultDecode('I am a string', 0)).toBe(true);


As indicated in #8611 (comment), please use toBeTruthy()/toBeFalsy() instead.

Also, please don't forget to squash the commits, see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

Snuffleupagus · 2017-07-05T13:01:35Z

test/unit/colorspace_spec.js

+    expect(ColorSpace.isDefaultDecode([0, 1], 1)).toBe(true);
+    expect(ColorSpace.isDefaultDecode([1, 0], 1)).toBe(false);
+    expect(ColorSpace.isDefaultDecode([1, 1], 1)).toBe(false);
+    expect(ColorSpace.isDefaultDecode([0, 1, 0, 1, 0, 1, 1, 1], 4)).toBe(false);


Did you mean for one of the last three cases to be truthy instead, since otherwise I'm not sure why you need so many (almost) identical test-cases?

No, they are meant to be falsy. I would change them, they look too verbose.

apoorv-mishra · 2017-07-05T13:34:46Z

@Snuffleupagus , I made the changes and squashed the commits. Please have a look and let me know.

apoorv-mishra · 2017-07-06T05:01:22Z

Hi @Snuffleupagus , let me know if the changes look fine so that I may proceed further with this work and add tests for other functions too. Thanks.

Snuffleupagus · 2017-07-06T10:29:17Z

let me know if the changes look fine so that I may proceed further with this work and add tests for other functions too. Thanks.

Since it's your first patch, and you explicitly asked for initial feedback, I provided some basic comments. However, I'm not sure if it's really necessary to review every small part piecewise at this point, so I'd suggest that we'll simply review it once you feel that the patch is complete :-)
One general piece of advice though: you've got a couple of merge commits which shouldn't be there (squash to get rid of them). I'd recommend that you use git rebase, rather than git merge, to avoid that in the future. Also, when you're updating the patch please change the commit message to "Add unit-tests for colorspace.js" instead.

As for what additional tests you need to add, I think that it would be nice to at least have a few simple unit-tests for e.g. the following methods: ColorSpace.parse (this will cover ColorSpace.parseToIR and ColorSpace.fromIR automatically), getRgb, getRgbItem, fillRgb.
I'd suggest searching for these methods in the src/core/*.js files, if you want examples of how they are used.

timvandermeij

Looks good so far! It's really good to have unit tests for this. I have added some comments about stylistic issues, so please resolve those to be consistent with the other parts of the codebase.

timvandermeij · 2017-07-27T20:49:42Z

test/unit/colorspace_spec.js

+ * limitations under the License.
+ */
+
+import {


Put these on one line since they fit within the 80 character limit.

timvandermeij · 2017-07-27T20:49:50Z

test/unit/colorspace_spec.js

+  Ref
+} from '../../src/core/primitives';
+import { Stream, StringStream } from '../../src/core/stream';
+import {


Put this on one line since they fit within the 80 character limit.

timvandermeij · 2017-07-27T20:51:03Z

test/unit/colorspace_spec.js

+      for (let elem in array) {
+        let obj = array[elem];
+        let ref = obj.ref,
+          data = obj.data;


Single lines should only contain one statement, so if you put this on a new line, make it let data = obj.data;.

timvandermeij · 2017-07-27T20:53:25Z

test/unit/colorspace_spec.js

+      expect(ColorSpace.isDefaultDecode('string', 0)).toBeTruthy();
+    });
+    it('should be true if length of decode array is not correct',
+      function () {


This applies to other places in the patch as well.

In the other parts of the codebase, we format this as follows, so please do that here as well:

it('should be true if length of decode array is not correct', function () { expect(ColorSpace.isDefaultDecode([0], 1)).toBeTruthy(); expect(ColorSpace.isDefaultDecode([0, 1, 0], 1)).toBeTruthy(); });

In other words, the function is intented with four spaces instead of two, while the body of the function is still indented with two spaces.

timvandermeij · 2017-07-27T20:55:32Z

test/unit/colorspace_spec.js

+
+      let testSrc = new Uint8Array([27, 125, 250, 131]);
+      let testDest = new Uint8Array(4 * 4 * 3);
+      let expectedDest = new Uint8Array(


This applies to other places in the patch as well.

Format this as follows:

let expectedDest = new Uint8Array([ 27, 27, 27, ..., 131, 131, 131, ]);

timvandermeij · 2017-07-27T20:57:15Z

test/unit/colorspace_spec.js

+
+      expect(colorSpace.getRgb(new Float32Array([0.2]), 0)).toEqual(
+        new Uint8Array(
+          [51, 51, 51]));


This applies to other places in the patch as well.

Move this back to the previous line.

timvandermeij · 2017-07-27T20:57:45Z

test/unit/colorspace_spec.js

+
+      let colorSpace = ColorSpace.parse(cs, xref, res);
+
+      let testSrc = new Uint8Array([27, 125, 250,


This applies to other places in the patch as well.

Put the part from the [ onward on the next line.

timvandermeij · 2017-07-27T21:02:15Z

test/unit/colorspace_spec.js

+      expect(testDest).toEqual(expectedDest);
+    });
+  });
+  describe('CalRGBCS', function () {


This applies to other places in the patch as well.

Please make sure that there is a newline before each describe block for readability.

timvandermeij · 2017-07-27T21:04:51Z

/botio unittest

pdfjsbot · 2017-07-27T21:04:52Z

From: Bot.io (Windows)

Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/0468a8a39ec70a2/output.txt

pdfjsbot · 2017-07-27T21:04:52Z

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/7d9ef8d6b32836a/output.txt

pdfjsbot · 2017-07-27T21:07:26Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/7d9ef8d6b32836a/output.txt

Total script time: 2.55 mins

Unit Tests: Passed

pdfjsbot · 2017-07-27T21:16:58Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/0468a8a39ec70a2/output.txt

Total script time: 12.10 mins

Unit Tests: Passed

apoorv-mishra · 2017-07-28T04:06:38Z

Hi @timvandermeij , I resolved the stylistic issues as mentioned. Please have a look.

Snuffleupagus

Besides a couple of quick inline comments, is there any reason why these tests wouldn't work if run on Node.js?
If not, please add colorspace_spec.js to the https://github.com/mozilla/pdf.js/blob/master/test/unit/clitests.json file.

Snuffleupagus · 2017-07-28T07:55:13Z

test/unit/colorspace_spec.js

+
+  describe('DeviceGrayCS', function () {
+    it('should handle the case when cs is a Name object', function () {
+      let cs = new Name('DeviceGray');


I'm going to comment on this once here, but it also applies in a number of places below:
Please use Name.get('DeviceGray') instead, since that only creates one instance of the Name object regardless of how many times it's called.

Snuffleupagus · 2017-07-28T07:55:30Z

test/unit/colorspace_spec.js

+      params.set('BlackPoint', [0, 0, 0]);
+      params.set('Gamma', 2.0);
+
+      let cs = new Array(2);


Here, and elsewhere too, using new Array isn't necessary. Please just do:

let cs = [ Name.get('CalGray'), params, ];

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues Fixed code-style issues Addressed issues pointed out in mozilla#8611 (review)

timvandermeij · 2017-07-28T20:16:43Z

/botio unittest

pdfjsbot · 2017-07-28T20:16:44Z

From: Bot.io (Linux m4)

Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/17bb92ab174b2f8/output.txt

pdfjsbot · 2017-07-28T20:16:44Z

From: Bot.io (Windows)

Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/a033a228a610cd9/output.txt

pdfjsbot · 2017-07-28T20:19:17Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/17bb92ab174b2f8/output.txt

Total script time: 2.55 mins

Unit Tests: Passed

pdfjsbot · 2017-07-28T20:29:06Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/a033a228a610cd9/output.txt

Total script time: 12.36 mins

Unit Tests: Passed

timvandermeij · 2017-07-28T21:30:19Z

/botio test

pdfjsbot · 2017-07-28T21:30:20Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/2c946f5b8ea04cb/output.txt

pdfjsbot · 2017-07-28T21:30:20Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.215.176.217:8877/66646a9eb6886a1/output.txt

pdfjsbot · 2017-07-28T21:46:53Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/2c946f5b8ea04cb/output.txt

Total script time: 16.54 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-07-28T21:59:22Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/66646a9eb6886a1/output.txt

Total script time: 29.02 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: Passed

timvandermeij · 2017-07-28T22:14:17Z

Thank you for providing the smoke tests!

(The one unit tests failure is unrelated to this patch and passed before.)

Added unit-tests for DeviceGray, DeviceRGB and DeviceCMYK Added unit-tests for CalGray Added unit-tests for CalRGB Removed redundant code Added unit-tests for LabCS Added unit-tests for IndexedCS Update comment Change lookup to Uint8Array as mentioned in pdf specs(these tests will pass after PR mozilla#8666 is merged). Added unit-tests for AlternateCS Resolved code-style issues Fixed code-style issues Addressed issues pointed out in mozilla#8611 (review)

Add unit-tests for colorspace.js

Snuffleupagus reviewed Jul 5, 2017

View reviewed changes

Snuffleupagus added test 4-work-in-progress labels Jul 5, 2017

Snuffleupagus reviewed Jul 5, 2017

View reviewed changes

apoorv-mishra force-pushed the colorspace-tests branch from 72dfc28 to 79fa82f Compare July 5, 2017 13:30

apoorv-mishra force-pushed the colorspace-tests branch 2 times, most recently from 60b2d52 to 016add5 Compare July 6, 2017 12:31

apoorv-mishra force-pushed the colorspace-tests branch from 016add5 to d6d9b99 Compare July 26, 2017 18:36

apoorv-mishra changed the title ~~Added tests for colorspace.js~~ Add unit-tests for colorspace.js Jul 26, 2017

timvandermeij removed the 4-work-in-progress label Jul 27, 2017

timvandermeij reviewed Jul 27, 2017

View reviewed changes

apoorv-mishra force-pushed the colorspace-tests branch from d6d9b99 to 86dedc1 Compare July 28, 2017 03:50

apoorv-mishra force-pushed the colorspace-tests branch from 86dedc1 to df0f0f5 Compare July 28, 2017 06:44

Snuffleupagus reviewed Jul 28, 2017

View reviewed changes

apoorv-mishra force-pushed the colorspace-tests branch from df0f0f5 to a129de7 Compare July 28, 2017 08:55

timvandermeij approved these changes Jul 28, 2017

View reviewed changes

timvandermeij merged commit 783d42e into mozilla:master Jul 28, 2017

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8611 from apoorv-mishra/colorspace-tests

8bd0219

Add unit-tests for colorspace.js


		let colorSpace = ColorSpace.parse(cs, xref, res);

		let testSrc = new Uint8Array([27, 125, 250,

Add unit-tests for colorspace.js #8611

Add unit-tests for colorspace.js #8611

Conversation

apoorv-mishra commented Jul 5, 2017 • edited by timvandermeij Loading

Snuffleupagus Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorv-mishra commented Jul 5, 2017

Snuffleupagus Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorv-mishra commented Jul 5, 2017

apoorv-mishra commented Jul 6, 2017

Snuffleupagus commented Jul 6, 2017

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jul 27, 2017

pdfjsbot commented Jul 27, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 27, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 27, 2017

From: Bot.io (Windows)

Success

apoorv-mishra commented Jul 28, 2017

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jul 28, 2017

pdfjsbot commented Jul 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 28, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 28, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 28, 2017

From: Bot.io (Windows)

Success

timvandermeij commented Jul 28, 2017

pdfjsbot commented Jul 28, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 28, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 28, 2017

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 28, 2017

From: Bot.io (Windows)

Failed

timvandermeij commented Jul 28, 2017 • edited Loading

apoorv-mishra commented Jul 5, 2017 •

edited by timvandermeij

Loading

Snuffleupagus Jul 5, 2017 •

edited

Loading

Snuffleupagus Jul 5, 2017 •

edited

Loading

Snuffleupagus Jul 5, 2017 •

edited

Loading

timvandermeij Jul 27, 2017 •

edited

Loading

timvandermeij commented Jul 28, 2017 •

edited

Loading