Unit test files as modules #7934

porlan1 · 2017-01-06T21:31:06Z

I changed the unit test files to UMD modules. I also changed the jasmine-boot.js file to load up the unit test modules and got rid of the lines that load up global variables. I also changed the unit_test.html file by removing the script tags for the unit test files, since they are now loaded in jasmine-boot.js

Fixes #7179

yurydelendik

Please squash into single commit

yurydelendik · 2017-01-06T21:38:12Z

test/unit/annotation_layer_spec.js

-           AnnotationBorderStyleType, StringStream, Lexer, Parser,
-           stringToUTF8String, AnnotationFieldFlag, PDFJS, stringToBytes */
-
+/* Copyright 2012 Mozilla Foundation


Use 2017 year.

yurydelendik · 2017-01-06T21:39:35Z

test/unit/annotation_layer_spec.js

+  if (typeof define === 'function' && define.amd) {
+    define('pdfjs-test/unit/annotation_layer_spec', ['exports', 'pdfjs/core/primitives','pdfjs/core/annotation','pdfjs/core/stream','pdfjs/core/parser','pdfjs/shared/util','pdfjs/display/global'], factory);
+  } else if (typeof exports !== 'undefined') {
+    factory(exports, require('../core/primitives.js'),require('../core/anotation.js'),require('../core/stream.js'),require('../core/parser.js'),require('../shared/util.js'),require('../display/global.js'));


The CommonJS modules are located at "../../src/core" and "../../src/display" also break down long lines at 80 chars

yurydelendik · 2017-01-06T21:41:01Z

test/unit/jasmine-boot.js

-          window[name] = lib[name];
-        });
-      });
+  require.config({paths: {'pdfjs': '../../src', 'pdfjs-web': '../../web','pdfjs-test':'../../test'}});


'pdfjs-test':'..'

Snuffleupagus

Please update the commit message to something a bit more descriptive than just "modularized", such that it's possible to understand what the patch does when looking at it e.g. from the Git command line.

Snuffleupagus · 2017-01-07T10:59:40Z

test/unit/annotation_layer_spec.js

+      root.pdfjsSharedUtil, root.pdfjsDisplayGlobal);
+  }
+}(this, function (exports, corePrimitives, coreAnnotation, coreStream,
+  coreParser, sharedUtil, displayGlobal) {


Nit: Please change the indentation of this line so that the parameter names line up, compare with e.g. this file; it should look like this:

}(this, function (exports, corePrimitives, coreAnnotation, coreStream, coreParser, sharedUtil, displayGlobal) {

Snuffleupagus · 2017-01-07T11:00:51Z

test/unit/evaluator_spec.js

+      root.pdfjsSharedUtil);
+  }
+}(this, function (exports, coreEvaluator, corePrimitives, coreStream,
+  coreWorker, sharedUtil) {


Nit: Please change the indentation of this line so that the parameter names line up, compare with e.g. this file; it should look like this:

}(this, function (exports, coreEvaluator, corePrimitives, coreStream, coreWorker, sharedUtil) {

Snuffleupagus · 2017-01-07T11:02:49Z

test/unit/jasmine-boot.js

+      testUnitFunctionSpec, testUnitMetadataSpec, testUnitMurmurHash3Spec,
+      testUnitNetworkSpec, testUnitParserSpec, testUnitPrimitivesSpec,
+      testUnitStreamSpec, testUnitType1ParserSpec, testUnitUiUtilsSpec,
+      testUnitUnicodeSpec, testUnitUtilSpec) {


Nit: Please change the indentation of this line so that the parameter names line up, compare with e.g. this file; it should look like this:

function (displayGlobal, testUnitAnnotationLayerSpec, testUnitApiSpec, testUnitCFFParserSpec, testUnitCMapSpec, testUnitCryptoSpec, testUnitDOMUtilsSpec, testUnitEvaluatorSpec, testUnitFontsSpec, testUnitFunctionSpec, testUnitMetadataSpec, testUnitMurmurHash3Spec, testUnitNetworkSpec, testUnitParserSpec, testUnitPrimitivesSpec, testUnitStreamSpec, testUnitType1ParserSpec, testUnitUiUtilsSpec, testUnitUnicodeSpec, testUnitUtilSpec) {

timvandermeij

Nice work!

timvandermeij · 2017-01-07T21:24:28Z

test/unit/unit_test.html

@@ -11,26 +11,6 @@
  <script src="testreporter.js"></script>
  <script src="jasmine-boot.js"></script>



Nit: remove this empty line as well

timvandermeij · 2017-01-08T14:32:45Z

/botio unittest

pdfjsbot · 2017-01-08T14:32:45Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/dff49b765a152db/output.txt

pdfjsbot · 2017-01-08T14:32:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/7df89dba73fafcd/output.txt

pdfjsbot · 2017-01-08T14:34:49Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/dff49b765a152db/output.txt

Total script time: 2.06 mins

Unit Tests: Passed

pdfjsbot · 2017-01-08T14:35:17Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/7df89dba73fafcd/output.txt

Total script time: 2.53 mins

Unit Tests: Passed

yurydelendik · 2017-01-09T14:27:32Z

Looks really good so far. Could you add the following changes to your patch and fix rest of the lint issues?

diff --git a/external/umdutils/verifier.js b/external/umdutils/verifier.js
index f5678fd..d763508 100644
--- a/external/umdutils/verifier.js
+++ b/external/umdutils/verifier.js
@@ -305,9 +305,30 @@ function validateFile(path, name, context) {
         parts.shift();
       }
       while (parts[0] === '..') {
+        if (base.length === 0) {
+          error('Invalid relative CommonJS path');
+        }
         parts.shift();
         base.pop();
       }
+      if (base.length === 0) {
+        // Reached the project root -- finding prefix matching subpath.
+        for (var prefix in context.paths) {
+          if (!context.paths.hasOwnProperty(prefix)) {
+            continue;
+          }
+          var prefixPath = context.paths[prefix];
+          if (!('./' + parts.join('/') + '/').startsWith(prefixPath + '/')) {
+            continue;
+          }
+          parts.splice(0, prefixPath.split('/').length - 1);
+          base.push(prefix);
+          break;
+        }
+        if (base.length === 0) {
+          error('Invalid relative CommonJS path prefix');
+        }
+      }
       if (j !== base.concat(parts).join('/')) {
         error('CommonJS path does not point to right AMD module: ' +
           i + ' vs ' + j);
@@ -473,6 +494,7 @@ function validateFiles(paths, options) {
     exports: Object.create(null),
     imports: Object.create(null),
     dependencies: Object.create(null),
+    paths: paths,
     errorCallback: errorCallback,
     warnCallback: warnCallback,
     infoCallback: infoCallback
diff --git a/gulpfile.js b/gulpfile.js
index 1cdc1c1..21367d7 100644
--- a/gulpfile.js
+++ b/gulpfile.js
@@ -541,7 +541,12 @@ gulp.task('lint', function (done) {
     console.log();
     console.log('### Checking UMD dependencies');
     var umd = require('./external/umdutils/verifier.js');
-    if (!umd.validateFiles({'pdfjs': './src', 'pdfjs-web': './web'})) {
+    var paths = {
+      'pdfjs': './src',
+      'pdfjs-web': './web',
+      'pdfjs-test': './test'
+    };
+    if (!umd.validateFiles(paths)) {
       done(new Error('UMD check failed.'));
       return;
     }
diff --git a/test/unit/crypto_spec.js b/test/unit/crypto_spec.js
index c07c754..7234689 100644
--- a/test/unit/crypto_spec.js
+++ b/test/unit/crypto_spec.js
@@ -23,7 +23,7 @@
             require('../../src/core/primitives.js'),
             require('../../src/shared/util.js'));
   } else {
-    factory((root.pdfjsTestCryptoSpec = {}), root.pdfjsCoreCrypto,
+    factory((root.pdfjsTestUnitCryptoSpec = {}), root.pdfjsCoreCrypto,
              root.pdfjsCorePrimitives, root.pdfjsSharedUtil);
   }
 }(this, function (exports, coreCrypto, corePrimitives, sharedUtil) {

yurydelendik · 2017-01-09T15:08:33Z

test/unit/annotation_layer_spec.js

-            require('../../src/core/parser.js'),
-            require('../../src/shared/util.js'),
-            require('../../src/display/global.js'));
+            require('pdfjs/core/anotation.js'),


I think it needs to be require('../../src/core/annotation.js'),

yurydelendik · 2017-01-09T16:13:48Z

/botio-linux unittest

pdfjsbot · 2017-01-09T16:13:48Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/8287dfd24cb9418/output.txt

pdfjsbot · 2017-01-09T16:15:48Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/8287dfd24cb9418/output.txt

Total script time: 2.00 mins

Unit Tests: Passed

porlan1 · 2017-01-09T16:17:13Z

Hi Yury, thanks for the help. Should I squash those commits?

yurydelendik · 2017-01-09T16:34:39Z

Should I squash those commits?

Yes, please

yurydelendik · 2017-01-09T16:48:18Z

/botio test

pdfjsbot · 2017-01-09T16:48:18Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/281588546298918/output.txt

pdfjsbot · 2017-01-09T16:48:18Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/6960a0640e47609/output.txt

pdfjsbot · 2017-01-09T17:13:49Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/6960a0640e47609/output.txt

Total script time: 25.52 mins

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

pdfjsbot · 2017-01-09T17:14:25Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/281588546298918/output.txt

Total script time: 26.11 mins

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

yurydelendik · 2017-01-09T17:27:27Z

Thank you for the patch.

timvandermeij · 2017-01-09T20:28:17Z

Nice contribution, thanks!

Unit test files as modules

yurydelendik requested changes Jan 6, 2017

View reviewed changes

porlan1 force-pushed the master branch from e21fe13 to 729633a Compare January 7, 2017 03:54

Snuffleupagus reviewed Jan 7, 2017

View reviewed changes

Snuffleupagus added the test label Jan 7, 2017

yurydelendik mentioned this pull request Jan 7, 2017

Replace global JS unit tests file with UMD modules #7179

Closed

porlan1 force-pushed the master branch from 44662dd to e762062 Compare January 7, 2017 16:04

timvandermeij reviewed Jan 7, 2017

View reviewed changes

porlan1 force-pushed the master branch from 25f96f0 to f6b6458 Compare January 7, 2017 23:15

yurydelendik approved these changes Jan 9, 2017

View reviewed changes

yurydelendik reviewed Jan 9, 2017

View reviewed changes

unit test files as UMD modules

d9e1cb7

porlan1 force-pushed the master branch from 1d5b3f1 to d9e1cb7 Compare January 9, 2017 16:41

yurydelendik merged commit 049d7fa into mozilla:master Jan 9, 2017

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

Merge pull request mozilla#7934 from porlan1/master

54e6f33

Unit test files as modules

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit test files as modules #7934

Unit test files as modules #7934

porlan1 commented Jan 6, 2017 •

edited by yurydelendik

Loading

yurydelendik left a comment

yurydelendik Jan 6, 2017

yurydelendik Jan 6, 2017

yurydelendik Jan 6, 2017

Snuffleupagus left a comment

Snuffleupagus Jan 7, 2017

Snuffleupagus Jan 7, 2017

Snuffleupagus Jan 7, 2017

timvandermeij left a comment

timvandermeij Jan 7, 2017

timvandermeij commented Jan 8, 2017

pdfjsbot commented Jan 8, 2017

pdfjsbot commented Jan 8, 2017

pdfjsbot commented Jan 8, 2017

pdfjsbot commented Jan 8, 2017

yurydelendik commented Jan 9, 2017 •

edited

Loading

yurydelendik Jan 9, 2017

yurydelendik commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

porlan1 commented Jan 9, 2017

yurydelendik commented Jan 9, 2017

yurydelendik commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

yurydelendik commented Jan 9, 2017

timvandermeij commented Jan 9, 2017

		@@ -11,26 +11,6 @@
		<script src="testreporter.js"></script>
		<script src="jasmine-boot.js"></script>

Unit test files as modules #7934

Unit test files as modules #7934

Conversation

porlan1 commented Jan 6, 2017 • edited by yurydelendik Loading

yurydelendik 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

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

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jan 8, 2017

pdfjsbot commented Jan 8, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 8, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 8, 2017

From: Bot.io (Linux)

Success

pdfjsbot commented Jan 8, 2017

From: Bot.io (Windows)

Success

yurydelendik commented Jan 9, 2017 • edited Loading

Choose a reason for hiding this comment

yurydelendik commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 9, 2017

From: Bot.io (Linux)

Success

porlan1 commented Jan 9, 2017

yurydelendik commented Jan 9, 2017

yurydelendik commented Jan 9, 2017

pdfjsbot commented Jan 9, 2017

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 9, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 9, 2017

From: Bot.io (Windows)

Success

pdfjsbot commented Jan 9, 2017

From: Bot.io (Linux)

Success

yurydelendik commented Jan 9, 2017

timvandermeij commented Jan 9, 2017

porlan1 commented Jan 6, 2017 •

edited by yurydelendik

Loading

yurydelendik commented Jan 9, 2017 •

edited

Loading