Stop excluding test modules when meteor.testModule found in package.j…

…son. (#10402)

New Meteor apps have the following meteor.testModule in their package.json
files by default

  "meteor": {
    "testModule": "tests/main.js"
  }

When meteor.testModule is defined, it determines the test entry point when
running the `meteor test` command, ignoring legacy file naming conventions
like *.tests.js or *.app-tests.js.

The package-source.js code changed by this commit was incorrect because it
ignored those specially-named test files even when running tests, which
was a problem if the meteor.testModule tried to import them explicitly,
because they would not be properly compiled.

If you're using meteor.testModule, the distinction between `meteor test`
and `meteor test --full-app` matters a bit less, since the test entry
point will be the same for both modes, though you can still check
Meteor.isTest and Meteor.isAppTest at runtime to control test behavior.

History.md

@@ -8,6 +8,12 @@ N/A
 
 ### Changes
 
+- Fixed a bug where modules named with `*.app-tests.js` (or `*.tests.js`)
+  file extensions sometimes could not be imported by the
+  `meteor.testModule` entry point when running the `meteor test` command
+  (or `meteor test --full-app`).
+  [PR #10402](https://github.com/meteor/meteor/pull/10402)
+
 ## v1.8.0.1, 2018-11-23
 
 ### Breaking changes

tools/isobuild/package-source.js

@@ -22,7 +22,8 @@ import { PackageAPI } from "./package-api.js";
 import {
   TEST_FILENAME_REGEXPS,
   APP_TEST_FILENAME_REGEXPS,
-  isTestFilePath } from './test-files.js';
+  isTestFilePath,
+} from './test-files.js';
 
 import {
   convert as convertColonsInPath
@@ -1102,15 +1103,6 @@ _.extend(PackageSource.prototype, {
     // Ignore the usual ignorable files.
     sourceReadOptions.exclude.push(...ignoreFiles);
 
-    // Unless we're running tests, ignore all test filenames and if we are, ignore the
-    // type of file we *aren't* running
-    if (!global.testCommandMetadata || global.testCommandMetadata.isTest) {
-      sourceReadOptions.exclude.push(...APP_TEST_FILENAME_REGEXPS);
-    }
-    if (!global.testCommandMetadata || global.testCommandMetadata.isAppTest) {
-      sourceReadOptions.exclude.push(...TEST_FILENAME_REGEXPS);
-    }
-
     // Read top-level source files, excluding control files that were not
     // explicitly included.
     const controlFiles = ['mobile-config.js'];
@@ -1121,11 +1113,48 @@ _.extend(PackageSource.prototype, {
 
     const anyLevelExcludes = [];
 
-    // If we have a meteor.testModule from package.json, then we don't
-    // need to exclude tests/ directories from the search, because we
-    // trust meteor.testModule to identify a single test entry point.
-    if (! testModule) {
+    if (testModule) {
+      // If we have a meteor.testModule from package.json, then we don't
+      // need to exclude tests/ directories or *.tests.js files from the
+      // search, because we trust meteor.testModule to identify a single
+      // test entry point.
+    } else {
       anyLevelExcludes.push(/^tests\/$/);
+
+      const {
+        isTest = false,
+        isAppTest = false,
+      } = global.testCommandMetadata || {};
+
+      if (isTest || isAppTest) {
+        // If we're running `meteor test` without the --full-app option,
+        // ignore app-test-only files like *.app-tests.js.
+        if (! isAppTest) {
+          sourceReadOptions.exclude.push(
+            ...APP_TEST_FILENAME_REGEXPS,
+          );
+        }
+
+        // If we're running `meteor test` with the --full-app option,
+        // ignore non-app-test files like *.tests.js. The wisdom of this
+        // behavior is debatable, since you might want to run non-app
+        // tests even when you're using the --full-app option, but it's
+        // legacy behavior at this point, and it doesn't matter if you're
+        // using meteor.testModule anyway (recommended).
+        if (! isTest) {
+          sourceReadOptions.exclude.push(
+            ...TEST_FILENAME_REGEXPS,
+          );
+        }
+
+      } else {
+        // If we're not running `meteor test` (and meteor.testModule is
+        // not defined in package.json), ignore all test files.
+        sourceReadOptions.exclude.push(
+          ...APP_TEST_FILENAME_REGEXPS,
+          ...TEST_FILENAME_REGEXPS,
+        );
+      }
     }
 
     anyLevelExcludes.push(

tools/tests/apps/modules/imports/imported.tests.js

@@ -0,0 +1,9 @@
+import assert from "assert";
+
+export const name = module.id.split("/").pop();
+
+describe(name, () => {
+  it("should be imported", () => {
+    assert.strictEqual(name, "imported.tests.js");
+  });
+});

tools/tests/apps/modules/tests.js

@@ -653,3 +653,10 @@ describe("circular package.json resolution chains", () => {
     );
   });
 });
+
+describe("imported *.tests.js modules", () => {
+  const { name } = require("./imports/imported.tests.js");
+  it("should be properly compiled", () => {
+    assert.strictEqual(name, "imported.tests.js");
+  });
+});

tools/tests/test-modes.js

@@ -17,17 +17,17 @@ selftest.define("'meteor test --port' accepts/rejects proper values", function (
   runAddPackage.expectExit(0);
 
   run = s.run("test", "--port", "3700", "--driver-package", "tmeasday:acceptance-test-driver");
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('App running at: http://localhost:3700/');
   run.stop();
 
   run = s.run("test", "--port", "127.0.0.1:3700", "--driver-package", "tmeasday:acceptance-test-driver");
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('App running at: http://127.0.0.1:3700/');
   run.stop();
 
   run = s.run("test", "--port", "[::]:3700", "--driver-package", "tmeasday:acceptance-test-driver");
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('App running at: http://[::]:3700/');
   run.stop();
 });
@@ -80,15 +80,15 @@ selftest.define("'meteor test' eagerly loads correct files", () => {
 
   // `meteor` should load app files, but not test files or app-test files
   run = s.run();
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('index.js');
   run.stop();
   run.forbid('foo.test.js');
   run.forbid('foo.app-test.js');
 
   // `meteor test` should load test files, but not app files or app-test files
   run = s.run('test', '--driver-package', 'tmeasday:acceptance-test-driver');
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('foo.test.js');
   run.stop();
   run.forbid('index.js');
@@ -102,9 +102,9 @@ selftest.define("'meteor test' eagerly loads correct files", () => {
     'tmeasday:acceptance-test-driver',
     '--full-app',
   );
-  run.waitSecs(120);
+  run.waitSecs(30);
   run.match('foo.app-test.js');
   run.match('index.js');
   run.stop();
   run.forbid('foo.test.js');  
-})
+});