From 1a9536baa2f55ebc8a199c8e1d46f7bbaa6e6416 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Sun, 16 Nov 2014 13:47:43 -0800 Subject: [PATCH] Fix test-packages See #3012, though. --- tools/catalog-local.js | 129 +++++++++++----------- tools/catalog.js | 5 - tools/commands.js | 230 ++++++++++++++++----------------------- tools/project-context.js | 88 +++++++++++++-- 4 files changed, 240 insertions(+), 212 deletions(-) diff --git a/tools/catalog-local.js b/tools/catalog-local.js index b5d49774e5..ee8f89e6af 100644 --- a/tools/catalog-local.js +++ b/tools/catalog-local.js @@ -25,17 +25,15 @@ var LocalCatalog = function (options) { // versionRecord} object. self.packages = {}; - // We use the initialization design pattern because it makes it easier to use - // both of our catalogs as singletons. self.initialized = false; self.containingCatalog = options.containingCatalog || self; // Local directories to search for package source trees self.localPackageSearchDirs = null; - // Packagedirs specified by addLocalPackage: added explicitly through a - // directory. We mainly use this to allow the user to run test-packages - // against a package in a specific directory. + // Package source trees added explicitly through a directory (not through a + // parent search directory). We mainly use this to allow the user to run + // test-packages against a package in a specific directory. self.explicitlyAddedLocalPackageDirs = []; // All packages found either by localPackageSearchDirs or @@ -74,6 +72,9 @@ _.extend(LocalCatalog.prototype, { // then we will ignore all versions of 'foo' that we find through the // package server. Directories that don't exist (or paths that aren't // directories) will be silently ignored. + // - explicitlyAddedLocalPackageDirs: an array of paths which THEMSELVES + // are package source trees. Takes precedence over packages found + // via localPackageSearchDirs. initialize: function (options) { var self = this; buildmessage.assertInCapture(); @@ -84,8 +85,12 @@ _.extend(LocalCatalog.prototype, { options.localPackageSearchDirs, function (p) { return path.resolve(p); }); + self.explicitlyAddedLocalPackageDirs = _.map( + options.explicitlyAddedLocalPackageDirs, function (p) { + return path.resolve(p); + }); - self._recomputeEffectiveLocalPackages(); + self._computeEffectiveLocalPackages(); self._loadLocalPackages(); self.initialized = true; }, @@ -154,44 +159,70 @@ _.extend(LocalCatalog.prototype, { return self.packages[name].versionRecord; }, + getVersionBySourceRoot: function (sourceRoot) { + var self = this; + var package = _.find(self.packages, function (p) { + return p.packageSource.sourceRoot === sourceRoot; + }); + if (! package) + return null; + return package.versionRecord; + }, + // Compute self.effectiveLocalPackageDirs from self.localPackageSearchDirs and // self.explicitlyAddedLocalPackageDirs. - _recomputeEffectiveLocalPackages: function () { + _computeEffectiveLocalPackages: function () { var self = this; - self.effectiveLocalPackageDirs = _.clone( - self.explicitlyAddedLocalPackageDirs); + buildmessage.assertInCapture(); - _.each(self.localPackageSearchDirs, function (searchDir) { - var possiblePackageDirs = watch.readAndWatchDirectory( - self.packageLocationWatchSet, { - absPath: searchDir, - include: [/\/$/] - }); - // Not a directory? Ignore. - if (possiblePackageDirs === null) - return; + self.effectiveLocalPackageDirs = []; - _.each(possiblePackageDirs, function (subdir) { - // readAndWatchDirectory adds a slash to the end of directory names to - // differentiate them from filenames. Remove it. - subdir = subdir.substr(0, subdir.length - 1); - var absPackageDir = path.join(searchDir, subdir); - - // Consider a directory to be a package source tree if it contains - // 'package.js'. (We used to support isopacks in localPackageSearchDirs, - // but no longer.) + buildmessage.enterJob("looking for packages", function () { + _.each(self.explicitlyAddedLocalPackageDirs, function (explicitDir) { var packageJs = watch.readAndWatchFile( - self.packageLocationWatchSet, - path.join(absPackageDir, 'package.js')); - if (packageJs !== null) { - // Let earlier package directories override later package - // directories. - - // We don't know the name of the package, so we can't deal with - // duplicates yet. We are going to have to rely on the fact that we - // are putting these in in order, to be processed in order. - self.effectiveLocalPackageDirs.push(absPackageDir); + self.packageLocationWatchSet, path.join(explicitDir, 'package.js')); + // We asked specifically for this directory, but it has no package! + if (packageJs === null) { + buildmessage.error("package has no package.js file", { + file: explicitDir + }); + return; // recover by ignoring } + self.effectiveLocalPackageDirs.push(explicitDir); + }); + + _.each(self.localPackageSearchDirs, function (searchDir) { + var possiblePackageDirs = watch.readAndWatchDirectory( + self.packageLocationWatchSet, { + absPath: searchDir, + include: [/\/$/] + }); + // Not a directory? Ignore. + if (possiblePackageDirs === null) + return; + + _.each(possiblePackageDirs, function (subdir) { + // readAndWatchDirectory adds a slash to the end of directory names to + // differentiate them from filenames. Remove it. + subdir = subdir.substr(0, subdir.length - 1); + var absPackageDir = path.join(searchDir, subdir); + + // Consider a directory to be a package source tree if it contains + // 'package.js'. (We used to support isopacks in + // localPackageSearchDirs, but no longer.) + var packageJs = watch.readAndWatchFile( + self.packageLocationWatchSet, + path.join(absPackageDir, 'package.js')); + if (packageJs !== null) { + // Let earlier package directories override later package + // directories. + + // We don't know the name of the package, so we can't deal with + // duplicates yet. We are going to have to rely on the fact that we + // are putting these in in order, to be processed in order. + self.effectiveLocalPackageDirs.push(absPackageDir); + } + }); }); }); }, @@ -291,30 +322,6 @@ _.extend(LocalCatalog.prototype, { }); }, - // Add a local package to the catalog. `name` is the name to use for - // the package and `directory` is the directory that contains the - // source tree for the package. - // - // If a package named `name` exists on the package server, it will - // be overridden (it will be as if that package doesn't exist on the - // package server at all). And for now, it's an error to call this - // function twice with the same `name`. - // XXX #3006 rewrite this - addLocalPackage: function (directory) { - var self = this; - buildmessage.assertInCapture(); - self._requireInitialized(); - - var resolvedPath = path.resolve(directory); - self.explicitlyAddedLocalPackageDirs.push(resolvedPath); - - // If we were making lots of calls to addLocalPackage, we would - // want to coalesce the calls to refresh somehow, but I don't - // think we'll actually be doing that so this should be fine. - // #CallingRefreshEveryTimeLocalPackagesChange - self.refresh(); - }, - getPackageSource: function (name) { var self = this; if (! _.has(self.packages, name)) diff --git a/tools/catalog.js b/tools/catalog.js index 88589d06f7..590f19a395 100644 --- a/tools/catalog.js +++ b/tools/catalog.js @@ -129,11 +129,6 @@ _.extend(LayeredCatalog.prototype, { self.otherCatalog = remote; }, - addLocalPackage: function (directory) { - var self = this; - self.localCatalog.addLocalPackage(directory); - }, - getLatestVersion: function (name) { var self = this; return self._returnFirst("getLatestVersion", arguments, ACCEPT_NON_EMPTY); diff --git a/tools/commands.js b/tools/commands.js index d60ac2801c..8cbd719aa1 100644 --- a/tools/commands.js +++ b/tools/commands.js @@ -1252,6 +1252,7 @@ main.registerCommand({ main.registerCommand({ name: 'test-packages', maxArgs: Infinity, + pretty: true, options: { port: { type: String, short: "p", default: DEFAULT_PORT }, 'http-proxy-port': { type: String }, @@ -1272,7 +1273,7 @@ main.registerCommand({ // doesn't do oplog tailing.) 'disable-oplog': { type: Boolean }, // Undocumented flag to use a different test driver. - 'driver-package': { type: String }, + 'driver-package': { type: String, default: 'test-in-browser' }, // Sets the path of where the temp app should be created 'test-app-path': { type: String }, @@ -1313,33 +1314,54 @@ main.registerCommand({ // XXX not good to change the options this way _.extend(options, parsedUrl); - var testPackages = getPackagesForTest(options.args); + // Find any packages mentioned by a path instead of a package name. We will + // load them explicitly into the catalog. + var packagesByPath = _.filter(options.args, function (p) { + return p.indexOf('/') !== -1; + }); // Make a temporary app dir (based on the test runner app). This will be // cleaned up on process exit. Using a temporary app dir means that we can // run multiple "test-packages" commands in parallel without them stomping // on each other. - // - // Note: testRunnerAppDir deliberately DOES NOT MATCH the app - // package search path baked into release.current.catalog: we are - // bundling the test runner app, but finding app packages from the - // current app (if any). var testRunnerAppDir = options['test-app-path'] || files.mkdtemp('meteor-test-run'); files.cp_r(path.join(__dirname, 'test-runner-app'), testRunnerAppDir); - // We are going to operate in the special test project, so let's remap our - // main project to the test directory. - project.setRootDir(testRunnerAppDir); - project.setMuted(true); // Mute output where applicable - // Hardset the proper release. - project.writeMeteorReleaseVersion(release.current.name || 'none'); - project.setDebug(!options.production); - project.forceEditPackages( - [options['driver-package'] || 'test-in-browser'], - 'add'); + // XXX Because every run uses a new app with its own IsopackCache directory, + // this always does a clean build of all packages. Maybe we can speed up + // repeated test-packages calls with some sort of shared or semi-shared + // isopack cache that's specific to test-packages? See #3012. + var projectContext = new projectContextModule.ProjectContext({ + projectDir: testRunnerAppDir, + // If we're currently in an app, we still want to use the real app's + // packages subdirectory, not the test runner app's empty one. + projectDirForLocalPackages: options.appDir, + explicitlyAddedLocalPackageDirs: packagesByPath + }); - var runners = []; + main.captureAndExit("=> Errors while setting up tests:", function () { + // Read metadata and initialize catalog. + projectContext.initializeCatalog(); + }); + + // Overwrite .meteor/release. + projectContext.releaseFile.write( + release.current.isCheckout() ? "none" : release.current.name); + + var packagesToAdd = getTestPackageNames(projectContext, options.args); + // Use the driver package as well. + packagesToAdd.push(options['driver-package']); + var constraintsToAdd = _.map(packagesToAdd, function (p) { + return utils.parseConstraint(p); + }); + // Add the packages to .meteor/packages. (We haven't yet resolved + // constraints, so this will affect constraint resolution.) + projectContext.projectConstraintsFile.addConstraints(constraintsToAdd); + + // The rest of the projectContext preparation process will happen inside the + // runner, once the proxy is listening. The changes we made were persisted to + // disk, so projectContext.reset won't make us forget anything. var mobileOptions = ['ios', 'ios-device', 'android', 'android-device']; var mobilePlatforms = []; @@ -1350,6 +1372,8 @@ main.registerCommand({ }); if (! _.isEmpty(mobilePlatforms)) { + var runners = []; + // XXX #3006 make sure this works // For this release; we won't force-enable the httpProxy if (false) { //!options.httpProxyPort) { console.log('Forcing http proxy on port 3002 for mobile'); @@ -1381,131 +1405,76 @@ main.registerCommand({ Console.stderr.write(err.message + '\n'); return 1; } + options.extraRunners = runners; } - options.extraRunners = runners; - return runTestAppForPackages(testPackages, testRunnerAppDir, options); + return runTestAppForPackages(projectContext, options); }); -// Ensures that packages are prepared and built for testing -var getPackagesForTest = function (packages) { - var testPackages; - if (packages.length === 0) { - // XXX #3006 no longer exists - testPackages = catalog.complete.getLocalPackageNames(); - } else { - var messages = buildmessage.capture(function () { - testPackages = _.map(packages, function (p) { - return buildmessage.enterJob({ - title: "Trying to test package `" + p + "`" - }, function () { - - // If it's a package name, just pass it through. - if (p.indexOf('/') === -1) { - if (p.indexOf('@') !== -1) { - buildmessage.error( - "You may not specify versions for local packages: " + p ); - // Recover by returning p anyway. - } - // Check to see if this is a real package, and if it is a real - // package, if it has tests. - // XXX #3006 no longer exists, and more in this functiona - if (!catalog.complete.isLocalPackage(p)) { - buildmessage.error( - "Not a known local package, cannot test: " + p ); - return p; - } - var versionNames = catalog.complete.getSortedVersions(p); - if (versionNames.length !== 1) - throw Error("local package should have one version?"); - var versionRec = catalog.complete.getVersion(p, versionNames[0]); - if (versionRec && !versionRec.testName) { - buildmessage.error( - "There are no tests for package: " + p ); - } - return p; +// Returns the "local-test:*" package names for the given package names (or for +// all local packages if packageNames is empty/unspecified). +var getTestPackageNames = function (projectContext, packageNames) { + var packageNamesSpecifiedExplicitly = ! _.isEmpty(packageNames); + if (_.isEmpty(packageNames)) { + // If none specified, test all local packages. (We don't have tests for + // non-local packages.) + packageNames = projectContext.localCatalog.getAllPackageNames(); + } + var testPackages = []; + main.captureAndExit("=> Errors while collecting tests:", function () { + _.each(packageNames, function (p) { + buildmessage.enterJob("trying to test package `" + p + "`", function () { + // If it's a package name, look it up the normal way. + if (p.indexOf('/') === -1) { + if (p.indexOf('@') !== -1) { + buildmessage.error( + "You may not specify versions for local packages: " + p ); + return; // recover by ignoring } - // Otherwise it's a directory; load it into a Package now. Use - // path.resolve to strip trailing slashes, so that packageName doesn't - // have a trailing slash. - // - // Why use addLocalPackage instead of just loading the packages - // and passing Isopack objects to the bundler? Because we - // actually need the Catalog to know about the package, so that - // we are able to resolve the test package's dependency on the - // main package. This is not ideal (I hate how this mutates global - // state) but it'll do for now. - var packageDir = path.resolve(p); - // XXX #3006 rework addLocalPackage - catalog.complete.addLocalPackage(packageDir); - - if (buildmessage.jobHasMessages()) { - // If we already had a problem, don't get another problem when we - // run the hack below. - return 'ignored'; + // Check to see if this is a real local package, and if it is a real + // local package, if it has tests. + var version = projectContext.localCatalog.getLatestVersion(p); + if (! version) { + buildmessage.error("Not a known local package, cannot test"); + } else if (version.testName) { + testPackages.push(version.testName); + } else if (packageNamesSpecifiedExplicitly) { + // It's only an error to *ask* to test a package with no tests, not + // to come across a package with no tests when you say "test all + // packages". + buildmessage.error("Package has no tests"); } - - // XXX: Hack. - var PackageSource = require('./package-source.js'); - var packageSource = new PackageSource(catalog.complete); - packageSource.initFromPackageDir(packageDir); - - return packageSource.name; - }); - + } else { + // Otherwise, it's a directory; find it by source root. + version = projectContext.localCatalog.getVersionBySourceRoot( + path.resolve(p)); + if (! version) { + throw Error("should have been caught when initializing catalog?"); + } + if (version.testName) { + testPackages.push(version.testName); + } else { + // This case only happens when explicitly asked for. + buildmessage.error("Package has no tests"); + } + } }); }); - - if (messages.hasMessages()) { - Console.stderr.write("\n" + messages.formatMessages()); - throw new main.ExitWithCode(1); - } - } + }); return testPackages; }; -var runTestAppForPackages = function (testPackages, testRunnerAppDir, options) { - // When we test packages, we need to know their versions and all of their - // dependencies. We are going to add them to the project and have the project - // compute them for us. This means that right now, we are testing all packages - // as they work together. - var tests = []; - _.each(testPackages, function(name) { - // XXX #3006 no longer exists, and more in this function - var versionNames = catalog.complete.getSortedVersions(name); - if (versionNames.length !== 1) - throw Error("local package should have one version?"); - var versionRecord = catalog.complete.getVersion(name, versionNames[0]); - if (versionRecord && versionRecord.testName) { - tests.push(versionRecord.testName); - } - }); - project.forceEditPackages(tests, 'add'); - - // We don't strictly need to do this before we bundle, but can't hurt. - var messages = buildmessage.capture({ - title: 'Getting packages ready' - },function () { - project._ensureDepsUpToDate(); - }); - - if (messages.hasMessages()) { - Console.stderr.write(messages.formatMessages()); - return 1; - } - - var webArchs = project.getWebArchs(); - +var runTestAppForPackages = function (projectContext, options) { var buildOptions = { minify: options.production, - webArchs: webArchs + includeDebug: ! options.production }; - var ret; if (options.deploy) { + // XXX #3006 when doing deploy, don't forget about this! buildOptions.serverArch = DEPLOY_ARCH; - ret = deploy.bundleAndDeploy({ + return deploy.bundleAndDeploy({ appDir: testRunnerAppDir, site: options.deploy, settingsFile: options.settings, @@ -1514,13 +1483,8 @@ var runTestAppForPackages = function (testPackages, testRunnerAppDir, options) { }); } else { var runAll = require('./run-all.js'); - ret = runAll.run(testRunnerAppDir, { - // if we're testing packages from an app, we still want to make - // sure the user doesn't 'meteor update' in the app, requiring - // a switch to a different release - // XXX #3006 instead of appDirForVersionCheck we should just - // use a ProjectContext whose local packages come - // from somewhere else + return runAll.run({ + projectContext: projectContext, proxyPort: options.port, httpProxyPort: options.httpProxyPort, debugPort: options['debug-port'], @@ -1538,8 +1502,6 @@ var runTestAppForPackages = function (testPackages, testRunnerAppDir, options) { extraRunners: options.extraRunners }); } - - return ret; }; /////////////////////////////////////////////////////////////////////////////// diff --git a/tools/project-context.js b/tools/project-context.js index b02290e235..85f6f49007 100644 --- a/tools/project-context.js +++ b/tools/project-context.js @@ -53,6 +53,13 @@ _.extend(exports.ProjectContext.prototype, { self._serverArchitectures.push(archinfo.host()); self._serverArchitectures = _.uniq(self._serverArchitectures); + // test-packages overrides this to load local packages from your real app + // instead of from test-runner-app. + self._projectDirForLocalPackages = options.projectDirForLocalPackages || + options.projectDir; + self._explicitlyAddedLocalPackageDirs = + options.explicitlyAddedLocalPackageDirs; + // Initialized by readProjectMetadata. self.releaseFile = null; self.projectConstraintsFile = null; @@ -285,7 +292,7 @@ _.extend(exports.ProjectContext.prototype, { _localPackageSearchDirs: function () { var self = this; - var searchDirs = [path.join(self.projectDir, 'packages')]; + var searchDirs = [path.join(self._projectDirForLocalPackages, 'packages')]; if (process.env.PACKAGE_DIRS) { // User can provide additional package directories to search in @@ -320,7 +327,10 @@ _.extend(exports.ProjectContext.prototype, { var searchDirs = self._localPackageSearchDirs(); buildmessage.enterJob({ title: "scanning local packages" }, function () { - self.projectCatalog.initialize({ localPackageSearchDirs: searchDirs }); + self.projectCatalog.initialize({ + localPackageSearchDirs: searchDirs, + explicitlyAddedLocalPackageDirs: self._explicitlyAddedLocalPackageDirs + }); if (buildmessage.jobHasMessages()) { self.projectCatalog = null; self.localCatalog = null; @@ -438,15 +448,15 @@ exports.ProjectConstraintsFile = function (options) { buildmessage.assertInCapture(); self.filename = path.join(options.projectDir, '.meteor', 'packages'); - self.watchSet = new watch.WatchSet; - // Maps from package name to parsed constraint. - self._constraints = null; + self.watchSet = null; // List of each line in the file; object with keys: // - leadingSpace (string of spaces before the constraint) // - constraint (as returned by utils.parseConstraint) // - trailingSpaceAndComment (string of spaces/comments after the constraint) // This allows us to rewrite the file preserving comments. self._constraintLines = null; + // Maps from package name to entry in _constraintLines. + self._constraintMap = null; self._readFile(); }; @@ -455,7 +465,8 @@ _.extend(exports.ProjectConstraintsFile.prototype, { var self = this; buildmessage.assertInCapture(); - self._constraints = {}; + self.watchSet = new watch.WatchSet; + self._constraintMap = {}; self._constraintLines = []; var contents = watch.readAndWatchFile(self.watchSet, self.filename); @@ -495,7 +506,7 @@ _.extend(exports.ProjectConstraintsFile.prototype, { } if (! lineRecord.constraint) return; // recover by ignoring - if (_.has(self._constraints, lineRecord.constraint.name)) { + if (_.has(self._constraintMap, lineRecord.constraint.name)) { buildmessage.error( "Package name appears twice: " + lineRecord.constraint.name, { // XXX should this be relative? @@ -503,16 +514,41 @@ _.extend(exports.ProjectConstraintsFile.prototype, { }); return; // recover by ignoring } - self._constraints[lineRecord.constraint.name] = lineRecord.constraint; + self._constraintMap[lineRecord.constraint.name] = lineRecord; }); }, + _write: function () { + var self = this; + var lines = _.map(self._constraintLines, function (lineRecord) { + var lineParts = [lineRecord.leadingSpace]; + if (lineRecord.constraint) { + lineParts.push(lineRecord.constraint.name); + if (lineRecord.constraint.constraintString) { + lineParts.push('@', lineRecord.constraint.constraintString); + } + } + lineParts.push(lineRecord.trailingSpaceAndComment); + return lineParts.join(''); + }); + files.writeFileAtomically(self.filename, lines.join('\n') + '\n'); + var messages = buildmessage.capture( + { title: 're-reading .meteor/packages' }, + function () { + self._readFile(); + }); + // We shouldn't choke on something we just wrote! + if (messages.hasMessages()) + throw Error("wrote bad .meteor/packages: " + messages.formatMessages()); + }, + // Iterates over all constraints, in the format returned by // utils.parseConstraint. eachConstraint: function (iterator) { var self = this; - _.each(self._constraints, function (constraint) { - iterator(constraint); + _.each(self._constraintLines, function (lineRecord) { + if (lineRecord.constraint) + iterator(lineRecord.constraint); }); }, @@ -520,9 +556,37 @@ _.extend(exports.ProjectConstraintsFile.prototype, { // null. getConstraint: function (name) { var self = this; - if (_.has(self._constraints, name)) - return self._constraints[name]; + if (_.has(self._constraintMap, name)) + return self._constraintMap[name].constraint; return null; + }, + + addConstraints: function (constraintsToAdd) { + var self = this; + var changed = false; + _.each(constraintsToAdd, function (constraintToAdd) { + var lineRecord; + if (! _.has(self._constraintMap, constraintToAdd.name)) { + lineRecord = { + leadingSpace: '', + constraint: constraintToAdd, + trailingSpaceAndComment: '' + }; + self._constraintLines.push(lineRecord); + self._constraintMap[constraintToAdd.name] = lineRecord; + changed = true; + return; + } + lineRecord = self._constraintMap[constraintToAdd.name]; + if (_.isEqual(constraintToAdd, lineRecord.constraint)) + return; // nothing changed + lineRecord.constraint = constraintToAdd; + changed = true; + }); + + if (! changed) + return; + self._write(); } });