From df2d46b33238f5de386915c03dc48976f9d87120 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween Date: Tue, 13 Aug 2019 08:36:10 -0400 Subject: [PATCH] Merge pull request #2074 from overleaf/em-o-error Introduce OError in CLSI manager GitOrigin-RevId: 4299de10a21242be4a127e761720de48f587412b --- .../app/src/Features/Compile/ClsiManager.js | 593 ++++++++++-------- .../test/unit/src/Compile/ClsiManagerTests.js | 334 +++++----- 2 files changed, 501 insertions(+), 426 deletions(-) diff --git a/services/web/app/src/Features/Compile/ClsiManager.js b/services/web/app/src/Features/Compile/ClsiManager.js index f92dd08435..18a596110f 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.js +++ b/services/web/app/src/Features/Compile/ClsiManager.js @@ -5,6 +5,8 @@ const ProjectGetter = require('../Project/ProjectGetter') const ProjectEntityHandler = require('../Project/ProjectEntityHandler') const logger = require('logger-sharelatex') const Url = require('url') +const OError = require('@overleaf/o-error') + const ClsiCookieManager = require('./ClsiCookieManager')( Settings.apis.clsi != null ? Settings.apis.clsi.backendGroupName : undefined ) @@ -20,54 +22,72 @@ const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandle const Metrics = require('metrics-sharelatex') const Errors = require('../Errors/Errors') +const VALID_COMPILERS = ['pdflatex', 'latex', 'xelatex', 'lualatex'] + const ClsiManager = { sendRequest(projectId, userId, options, callback) { if (options == null) { options = {} } - ClsiManager.sendRequestOnce(projectId, userId, options, function( - error, - status, - ...result - ) { - if (error != null) { - return callback(error) + ClsiManager.sendRequestOnce( + projectId, + userId, + options, + (err, status, ...result) => { + if (err != null) { + return callback(err) + } + if (status === 'conflict') { + // Try again, with a full compile + return ClsiManager.sendRequestOnce( + projectId, + userId, + { ...options, syncType: 'full' }, + callback + ) + } + callback(null, status, ...result) } - if (status === 'conflict') { - options = _.clone(options) - options.syncType = 'full' // force full compile - ClsiManager.sendRequestOnce(projectId, userId, options, callback) // try again - } else { - callback(error, status, ...result) - } - }) + ) }, sendRequestOnce(projectId, userId, options, callback) { if (options == null) { options = {} } - ClsiManager._buildRequest(projectId, options, function(error, req) { - if (error != null) { - if (error.message === 'no main file specified') { + ClsiManager._buildRequest(projectId, options, (err, req) => { + if (err != null) { + if (err.message === 'no main file specified') { return callback(null, 'validation-problems', null, null, { - mainFile: error.message + mainFile: err.message }) } else { - return callback(error) + return callback( + new OError({ + message: 'Could not build request to CLSI', + info: { projectId, options } + }).withCause(err) + ) } } logger.log({ projectId }, 'sending compile to CLSI') - ClsiManager._sendBuiltRequest(projectId, userId, req, options, function( - error, - status, - ...result - ) { - if (error != null) { - return callback(error) + ClsiManager._sendBuiltRequest( + projectId, + userId, + req, + options, + (err, status, ...result) => { + if (err != null) { + return callback( + new OError({ + message: 'CLSI compile failed', + info: { projectId, userId, req, options } + }).withCause(err) + ) + } + callback(null, status, ...result) } - callback(error, status, ...result) - }) + ) }) }, @@ -86,18 +106,26 @@ const ClsiManager = { null, clsiRequest, options, - function(error, status, ...result) { - if (error != null) { - return callback(error) + (err, status, ...result) => { + if (err != null) { + return callback( + new OError({ + message: 'CLSI compile failed', + info: { submissionId, clsiRequest, options } + }).withCause(err) + ) } - callback(error, status, ...result) + callback(null, status, ...result) } ) }, stopCompile(projectId, userId, options, callback) { + if (options == null) { + options = {} + } const compilerUrl = this._getCompilerUrl( - options != null ? options.compileGroup : undefined, + options.compileGroup, projectId, userId, 'compile/stop' @@ -110,8 +138,11 @@ const ClsiManager = { }, deleteAuxFiles(projectId, userId, options, callback) { + if (options == null) { + options = {} + } const compilerUrl = this._getCompilerUrl( - options != null ? options.compileGroup : undefined, + options.compileGroup, projectId, userId ) @@ -119,19 +150,29 @@ const ClsiManager = { url: compilerUrl, method: 'DELETE' } - ClsiManager._makeRequest(projectId, opts, clsiError => + ClsiManager._makeRequest(projectId, opts, clsiErr => { // always clear the project state from the docupdater, even if there // was a problem with the request to the clsi - DocumentUpdaterHandler.clearProjectState(projectId, function( - docUpdaterError - ) { - const error = clsiError || docUpdaterError - if (error != null) { - return callback(error) + DocumentUpdaterHandler.clearProjectState(projectId, docUpdaterErr => { + if (clsiErr != null) { + return callback( + new OError({ + message: 'Error making request to CLSI', + info: { projectId, opts } + }).withCause(clsiErr) + ) + } + if (docUpdaterErr != null) { + return callback( + new OError({ + message: 'Failed to clear project state in doc updater', + info: { projectId } + }).withCause(docUpdaterErr) + ) } callback() }) - ) + }) }, _sendBuiltRequest(projectId, userId, req, options, callback) { @@ -140,14 +181,15 @@ const ClsiManager = { } ClsiFormatChecker.checkRecoursesForProblems( req.compile != null ? req.compile.resources : undefined, - function(err, validationProblems) { + (err, validationProblems) => { if (err != null) { - logger.warn( - err, - projectId, - 'could not check resources for potential problems before sending to clsi' + return callback( + new OError({ + message: + 'could not check resources for potential problems before sending to clsi', + info: { req } + }).withCause(err) ) - return callback(err) } if (validationProblems != null) { logger.log( @@ -167,13 +209,14 @@ const ClsiManager = { userId, req, options.compileGroup, - function(error, response) { - if (error != null) { - logger.warn( - { err: error, projectId }, - 'error sending request to clsi' + (err, response) => { + if (err != null) { + return callback( + new OError({ + message: 'error sending request to clsi', + info: { projectId, userId, req, options } + }).withCause(err) ) - return callback(error) } if (response != null) { logger.log( @@ -187,13 +230,14 @@ const ClsiManager = { 'received compile response from CLSI' ) } - ClsiCookieManager._getServerId(projectId, function( - err, - clsiServerId - ) { + ClsiCookieManager._getServerId(projectId, (err, clsiServerId) => { if (err != null) { - logger.warn({ err, projectId }, 'error getting server id') - return callback(err) + return callback( + new OError({ + message: 'error getting server id', + info: { projectId } + }).withCause(err) + ) } const outputFiles = ClsiManager._parseOutputFiles( projectId, @@ -217,32 +261,42 @@ const ClsiManager = { { currentBackend(cb) { const startTime = new Date() - ClsiCookieManager.getCookieJar(projectId, function(err, jar) { + ClsiCookieManager.getCookieJar(projectId, (err, jar) => { if (err != null) { - logger.warn({ err }, 'error getting cookie jar for clsi request') - return callback(err) + return callback( + new OError({ + message: 'error getting cookie jar for CLSI request', + info: { projectId } + }).withCause(err) + ) } opts.jar = jar const timer = new Metrics.Timer('compile.currentBackend') - request(opts, function(err, response, body) { + request(opts, (err, response, body) => { + if (err != null) { + return callback( + new OError({ + message: 'error making request to CLSI', + info: { projectId, opts } + }).withCause(err) + ) + } timer.done() Metrics.inc( - `compile.currentBackend.response.${ - response != null ? response.statusCode : undefined - }` + `compile.currentBackend.response.${response.statusCode}` ) - if (err != null) { - logger.warn( - { err, projectId, url: opts != null ? opts.url : undefined }, - 'error making request to clsi' - ) - return callback(err) - } - ClsiCookieManager.setServerId(projectId, response, function(err) { + ClsiCookieManager.setServerId(projectId, response, err => { if (err != null) { - logger.warn({ err, projectId }, 'error setting server id') + callback( + new OError({ + message: 'error setting server id', + info: { projectId } + }).withCause(err) + ) + } else { + // return as soon as the standard compile has returned + callback(null, response, body) } - callback(err, response, body) // return as soon as the standard compile has returned cb(err, { response, body, @@ -254,108 +308,99 @@ const ClsiManager = { }, newBackend(cb) { const startTime = new Date() - ClsiManager._makeNewBackendRequest(projectId, opts, function( - err, - response, - body - ) { - Metrics.inc( - `compile.newBackend.response.${ - response != null ? response.statusCode : undefined - }` - ) - cb(err, { - response, - body, - finishTime: new Date() - startTime - }) - }) + ClsiManager._makeNewBackendRequest( + projectId, + opts, + (err, response, body) => { + if (err != null) { + logger.warn({ err }, 'Error making request to new CLSI backend') + } + if (response != null) { + Metrics.inc( + `compile.newBackend.response.${response.statusCode}` + ) + } + cb(err, { + response, + body, + finishTime: new Date() - startTime + }) + } + ) } }, - function(err, results) { + (err, results) => { if (err != null) { - logger.warn({ err }, 'Error making request to CLSI') + // This was handled higher up return } - const timeDifference = - (results.newBackend != null - ? results.newBackend.finishTime - : undefined) - - (results.currentBackend != null - ? results.currentBackend.finishTime - : undefined) - const newStatusCode = - results.newBackend && - results.newBackend.response && - results.newBackend.response.statusCode - const currentStatusCode = - results.currentBackend && - results.currentBackend.response && - results.currentBackend.response.statusCode - const statusCodeSame = newStatusCode === currentStatusCode - const currentCompileTime = - results.currentBackend != null - ? results.currentBackend.finishTime - : undefined - const newBackendCompileTime = - results.newBackend != null ? results.newBackend.finishTime : undefined - logger.log( - { - statusCodeSame, - timeDifference, - currentCompileTime, - newBackendCompileTime, - projectId - }, - 'both clsi requests returned' - ) + if (results.newBackend != null && results.newBackend.response != null) { + const currentStatusCode = results.currentBackend.response.statusCode + const newStatusCode = results.newBackend.response.statusCode + const statusCodeSame = newStatusCode === currentStatusCode + const currentCompileTime = results.currentBackend.finishTime + const newBackendCompileTime = results.newBackend.finishTime || 0 + const timeDifference = newBackendCompileTime - currentCompileTime + logger.log( + { + statusCodeSame, + timeDifference, + currentCompileTime, + newBackendCompileTime, + projectId + }, + 'both clsi requests returned' + ) + } } ) }, _makeNewBackendRequest(projectId, baseOpts, callback) { - if ( - (Settings.apis.clsi_new != null - ? Settings.apis.clsi_new.url - : undefined) == null - ) { + if (Settings.apis.clsi_new == null || Settings.apis.clsi_new.url == null) { return callback() } - const opts = _.clone(baseOpts) - opts.url = opts.url.replace( - Settings.apis.clsi.url, - Settings.apis.clsi_new != null ? Settings.apis.clsi_new.url : undefined - ) - NewBackendCloudClsiCookieManager.getCookieJar(projectId, function( - err, - jar - ) { + const opts = { + ...baseOpts, + url: baseOpts.url.replace( + Settings.apis.clsi.url, + Settings.apis.clsi_new.url + ) + } + NewBackendCloudClsiCookieManager.getCookieJar(projectId, (err, jar) => { if (err != null) { - logger.warn({ err }, 'error getting cookie jar for clsi request') - return callback(err) + return callback( + new OError({ + message: 'error getting cookie jar for CLSI request', + info: { projectId } + }).withCause(err) + ) } opts.jar = jar const timer = new Metrics.Timer('compile.newBackend') - request(opts, function(err, response, body) { + request(opts, (err, response, body) => { timer.done() if (err != null) { - logger.warn( - { err, projectId, url: opts != null ? opts.url : undefined }, - 'error making request to new clsi' + return callback( + new OError({ + message: 'error making request to new CLSI', + info: { projectId, opts } + }).withCause(err) ) - return callback(err) } NewBackendCloudClsiCookieManager.setServerId( projectId, response, - function(err) { + err => { if (err != null) { - logger.warn( - { err, projectId }, - 'error setting server id new backend' + return callback( + new OError({ + message: 'error setting server id on new backend', + info: { projectId } + }).withCause(err) ) } - callback(err, response, body) + callback(null, response, body) } ) }) @@ -386,9 +431,9 @@ const ClsiManager = { json: req, method: 'POST' } - ClsiManager._makeRequest(projectId, opts, function(error, response, body) { - if (error != null) { - return callback(error) + ClsiManager._makeRequest(projectId, opts, (err, response, body) => { + if (err != null) { + return callback(err) } if (response.statusCode >= 200 && response.statusCode < 300) { callback(null, body) @@ -399,19 +444,17 @@ const ClsiManager = { } else if (response.statusCode === 423) { callback(null, { compile: { status: 'compile-in-progress' } }) } else { - error = new Error( - `CLSI returned non-success code: ${response.statusCode}` + callback( + new OError({ + message: `CLSI returned non-success code: ${response.statusCode}`, + info: { projectId, opts, body } + }) ) - logger.warn({ err: error, projectId }, 'CLSI returned failure code') - callback(error, body) } }) }, - _parseOutputFiles(projectId, rawOutputFiles) { - if (rawOutputFiles == null) { - rawOutputFiles = [] - } + _parseOutputFiles(projectId, rawOutputFiles = []) { const outputFiles = [] for (const file of rawOutputFiles) { outputFiles.push({ @@ -424,8 +467,6 @@ const ClsiManager = { return outputFiles }, - VALID_COMPILERS: ['pdflatex', 'latex', 'xelatex', 'lualatex'], - _buildRequest(projectId, options, callback) { if (options == null) { options = {} @@ -433,34 +474,35 @@ const ClsiManager = { ProjectGetter.getProject( projectId, { compiler: 1, rootDoc_id: 1, imageName: 1, rootFolder: 1 }, - function(error, project) { - let timer - if (error != null) { - return callback(error) + (err, project) => { + if (err != null) { + return callback( + new OError({ + message: 'failed to get project', + info: { projectId } + }).withCause(err) + ) } if (project == null) { return callback( new Errors.NotFoundError(`project does not exist: ${projectId}`) ) } - if (!ClsiManager.VALID_COMPILERS.includes(project.compiler)) { + if (!VALID_COMPILERS.includes(project.compiler)) { project.compiler = 'pdflatex' } if (options.incrementalCompilesEnabled || options.syncType != null) { // new way, either incremental or full - timer = new Metrics.Timer('editor.compile-getdocs-redis') + const timer = new Metrics.Timer('editor.compile-getdocs-redis') ClsiManager.getContentFromDocUpdaterIfMatch( projectId, project, options, - function(error, projectStateHash, docUpdaterDocs) { + (err, projectStateHash, docUpdaterDocs) => { timer.done() - if (error != null) { - logger.error( - { err: error, projectId }, - 'error checking project state' - ) + if (err != null) { + logger.error({ err, projectId }, 'error checking project state') // note: we don't bail out when there's an error getting // incremental files from the docupdater, we just fall back // to a normal compile below @@ -478,7 +520,7 @@ const ClsiManager = { if ( docUpdaterDocs != null && options.syncType !== 'full' && - error == null + err == null ) { Metrics.inc('compile-from-redis') ClsiManager._buildRequestFromDocupdater( @@ -503,15 +545,16 @@ const ClsiManager = { ) } else { // old way, always from mongo - timer = new Metrics.Timer('editor.compile-getdocs-mongo') - ClsiManager._getContentFromMongo(projectId, function( - error, - docs, - files - ) { + const timer = new Metrics.Timer('editor.compile-getdocs-mongo') + ClsiManager._getContentFromMongo(projectId, (err, docs, files) => { timer.done() - if (error != null) { - return callback(error) + if (err != null) { + return callback( + new OError({ + message: 'failed to get contents from Mongo', + info: { projectId } + }).withCause(err) + ) } ClsiManager._finaliseRequest( projectId, @@ -528,19 +571,26 @@ const ClsiManager = { }, getContentFromDocUpdaterIfMatch(projectId, project, options, callback) { - ClsiStateManager.computeHash(project, options, function( - error, - projectStateHash - ) { - if (error != null) { - return callback(error) + ClsiStateManager.computeHash(project, options, (err, projectStateHash) => { + if (err != null) { + return callback( + new OError({ + message: 'Failed to compute project state hash', + info: { projectId } + }).withCause(err) + ) } DocumentUpdaterHandler.getProjectDocsIfMatch( projectId, projectStateHash, - function(error, docs) { - if (error != null) { - return callback(error) + (err, docs) => { + if (err != null) { + return callback( + new OError({ + message: 'Failed to get project documents', + info: { projectId, projectStateHash } + }).withCause(err) + ) } callback(null, projectStateHash, docs) } @@ -552,9 +602,14 @@ const ClsiManager = { const url = `${ Settings.apis.clsi.url }/project/${projectId}/user/${userId}/build/${buildId}/output/${outputFilePath}` - ClsiCookieManager.getCookieJar(projectId, function(err, jar) { + ClsiCookieManager.getCookieJar(projectId, (err, jar) => { if (err != null) { - return callback(err) + return callback( + new OError({ + message: 'Failed to get cookie jar', + info: { projectId, userId, buildId, outputFilePath } + }).withCause(err) + ) } const options = { url, method: 'GET', timeout: 60 * 1000, jar } const readStream = request(options) @@ -570,17 +625,18 @@ const ClsiManager = { docUpdaterDocs, callback ) { - ProjectEntityHandler.getAllDocPathsFromProject(project, function( - error, - docPath - ) { - let path - if (error != null) { - return callback(error) + ProjectEntityHandler.getAllDocPathsFromProject(project, (err, docPath) => { + if (err != null) { + return callback( + new OError({ + message: 'Failed to get doc paths', + info: { projectId } + }).withCause(err) + ) } const docs = {} for (let doc of docUpdaterDocs || []) { - path = docPath[doc._id] + const path = docPath[doc._id] docs[path] = doc } // send new docs but not files as those are already on the clsi @@ -593,7 +649,7 @@ const ClsiManager = { const possibleRootDocIds = [options.rootDoc_id, project.rootDoc_id] for (const rootDocId of possibleRootDocIds) { if (rootDocId != null && rootDocId in docPath) { - path = docPath[rootDocId] + const path = docPath[rootDocId] if (docs[path] == null) { docs[path] = { _id: rootDocId, path } } @@ -617,13 +673,20 @@ const ClsiManager = { projectStateHash, callback ) { - ClsiManager._getContentFromMongo(projectId, function(error, docs, files) { - if (error != null) { - return callback(error) + ClsiManager._getContentFromMongo(projectId, (err, docs, files) => { + if (err != null) { + return callback( + new OError({ + message: 'failed to get project contents from Mongo', + info: { projectId } + }).withCause(err) + ) + } + options = { + ...options, + syncType: 'full', + syncState: projectStateHash } - options = _.clone(options) - options.syncType = 'full' - options.syncState = projectStateHash ClsiManager._finaliseRequest( projectId, options, @@ -636,40 +699,51 @@ const ClsiManager = { }, _getContentFromMongo(projectId, callback) { - DocumentUpdaterHandler.flushProjectToMongo(projectId, function(error) { - if (error != null) { - return callback(error) + DocumentUpdaterHandler.flushProjectToMongo(projectId, err => { + if (err != null) { + return callback( + new OError({ + message: 'failed to flush project to Mongo', + info: { projectId } + }).withCause(err) + ) } - ProjectEntityHandler.getAllDocs(projectId, function(error, docs) { - if (docs == null) { - docs = {} + ProjectEntityHandler.getAllDocs(projectId, (err, docs) => { + if (err != null) { + return callback( + new OError({ + message: 'failed to get project docs', + info: { projectId } + }).withCause(err) + ) } - if (error != null) { - return callback(error) - } - ProjectEntityHandler.getAllFiles(projectId, function(error, files) { + ProjectEntityHandler.getAllFiles(projectId, (err, files) => { + if (err != null) { + return callback( + new OError({ + message: 'failed to get project files', + info: { projectId } + }).withCause(err) + ) + } if (files == null) { files = {} } - if (error != null) { - return callback(error) - } - callback(null, docs, files) + callback(null, docs || {}, files || {}) }) }) }) }, _finaliseRequest(projectId, options, project, docs, files, callback) { - let doc, path const resources = [] let rootResourcePath = null let rootResourcePathOverride = null let hasMainFile = false let numberOfDocsInProject = 0 - for (path in docs) { - doc = docs[path] + for (let path in docs) { + const doc = docs[path] path = path.replace(/^\//, '') // Remove leading / numberOfDocsInProject++ if (doc.lines != null) { @@ -708,20 +782,25 @@ const ClsiManager = { rootResourcePath = 'main.tex' } else if (numberOfDocsInProject === 1) { // only one file, must be the main document - for (path in docs) { - doc = docs[path] + for (const path in docs) { + // Remove leading / rootResourcePath = path.replace(/^\//, '') - } // Remove leading / + } logger.warn( { projectId, rootResourcePath }, 'no root document found, single document in project' ) } else { - return callback(new Error('no main file specified')) + return callback( + new OError({ + message: 'no main file specified', + info: { projectId } + }) + ) } } - for (path in files) { + for (let path in files) { const file = files[path] path = path.replace(/^\//, '') // Remove leading / resources.push({ @@ -751,13 +830,18 @@ const ClsiManager = { }, wordCount(projectId, userId, file, options, callback) { - ClsiManager._buildRequest(projectId, options, function(error, req) { - if (error != null) { - return callback(error) + ClsiManager._buildRequest(projectId, options, (err, req) => { + if (err != null) { + return callback( + new OError({ + message: 'Failed to build CLSI request', + info: { projectId, options } + }).withCause(err) + ) } const filename = file || req.compile.rootResourcePath const wordCountUrl = ClsiManager._getCompilerUrl( - options != null ? options.compileGroup : undefined, + options.compileGroup, projectId, userId, 'wordcount' @@ -770,22 +854,29 @@ const ClsiManager = { }, method: 'GET' } - ClsiManager._makeRequest(projectId, opts, function( - error, - response, - body - ) { - if (error != null) { - return callback(error) + ClsiManager._makeRequest(projectId, opts, (err, response, body) => { + if (err != null) { + return callback( + new OError({ + message: 'CLSI request failed', + info: { projectId, opts } + }).withCause(err) + ) } if (response.statusCode >= 200 && response.statusCode < 300) { callback(null, body) } else { - error = new Error( - `CLSI returned non-success code: ${response.statusCode}` + callback( + new OError({ + message: `CLSI returned non-success code: ${response.statusCode}`, + info: { + projectId, + opts, + response: body, + statusCode: response.statusCode + } + }) ) - logger.warn({ err: error, projectId }, 'CLSI returned failure code') - callback(error, body) } }) }) diff --git a/services/web/test/unit/src/Compile/ClsiManagerTests.js b/services/web/test/unit/src/Compile/ClsiManagerTests.js index 8d2ac02862..8409b2ce66 100644 --- a/services/web/test/unit/src/Compile/ClsiManagerTests.js +++ b/services/web/test/unit/src/Compile/ClsiManagerTests.js @@ -1,27 +1,11 @@ -/* eslint-disable - handle-callback-err, - max-len, - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * DS206: Consider reworking classes to avoid initClass - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const chai = require('chai') -const should = chai.should() const { expect } = chai const modulePath = '../../../../app/src/Features/Compile/ClsiManager.js' const SandboxedModule = require('sandboxed-module') describe('ClsiManager', function() { beforeEach(function() { - let Timer this.jar = { cookie: 'stuff' } this.ClsiCookieManager = { getCookieJar: sinon.stub().callsArgWith(1, null, this.jar), @@ -34,6 +18,27 @@ describe('ClsiManager', function() { this.ClsiFormatChecker = { checkRecoursesForProblems: sinon.stub().callsArgWith(1) } + this.Project = {} + this.ProjectEntityHandler = {} + this.ProjectGetter = {} + this.DocumentUpdaterHandler = { + getProjectDocsIfMatch: sinon.stub().callsArgWith(2, null, null) + } + this.logger = { + log: sinon.stub(), + error: sinon.stub(), + err: sinon.stub(), + warn: sinon.stub() + } + this.request = sinon.stub() + this.Metrics = { + Timer: class Metrics { + constructor() { + this.done = sinon.stub() + } + }, + inc: sinon.stub() + } this.ClsiManager = SandboxedModule.require(modulePath, { globals: { console: console @@ -54,40 +59,23 @@ describe('ClsiManager', function() { } }), '../../models/Project': { - Project: (this.Project = {}) + Project: this.Project }, - '../Project/ProjectEntityHandler': (this.ProjectEntityHandler = {}), - '../Project/ProjectGetter': (this.ProjectGetter = {}), - '../DocumentUpdater/DocumentUpdaterHandler': (this.DocumentUpdaterHandler = { - getProjectDocsIfMatch: sinon.stub().callsArgWith(2, null, null) - }), + '../Project/ProjectEntityHandler': this.ProjectEntityHandler, + '../Project/ProjectGetter': this.ProjectGetter, + '../DocumentUpdater/DocumentUpdaterHandler': this + .DocumentUpdaterHandler, './ClsiCookieManager': () => this.ClsiCookieManager, './ClsiStateManager': this.ClsiStateManager, - 'logger-sharelatex': (this.logger = { - log: sinon.stub(), - error: sinon.stub(), - err: sinon.stub(), - warn: sinon.stub() - }), - request: (this.request = sinon.stub()), + 'logger-sharelatex': this.logger, + request: this.request, './ClsiFormatChecker': this.ClsiFormatChecker, - 'metrics-sharelatex': (this.Metrics = { - Timer: (Timer = (function() { - Timer = class Timer { - static initClass() { - this.prototype.done = sinon.stub() - } - } - Timer.initClass() - return Timer - })()), - inc: sinon.stub() - }) + 'metrics-sharelatex': this.Metrics } }) this.project_id = 'project-id' this.user_id = 'user-id' - return (this.callback = sinon.stub()) + this.callback = sinon.stub() }) describe('sendRequest', function() { @@ -95,7 +83,7 @@ describe('ClsiManager', function() { this.ClsiManager._buildRequest = sinon .stub() .callsArgWith(2, null, (this.request = 'mock-request')) - return this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') }) describe('with a successful compile', function() { @@ -123,7 +111,7 @@ describe('ClsiManager', function() { ] } }) - return this.ClsiManager.sendRequest( + this.ClsiManager.sendRequest( this.project_id, this.user_id, { compileGroup: 'standard' }, @@ -132,13 +120,13 @@ describe('ClsiManager', function() { }) it('should build the request', function() { - return this.ClsiManager._buildRequest + this.ClsiManager._buildRequest .calledWith(this.project_id) .should.equal(true) }) it('should send the request to the CLSI', function() { - return this.ClsiManager._postToClsi + this.ClsiManager._postToClsi .calledWith(this.project_id, this.user_id, this.request, 'standard') .should.equal(true) }) @@ -162,7 +150,7 @@ describe('ClsiManager', function() { build: 1234 } ] - return this.callback + this.callback .calledWith(null, this.status, outputFiles) .should.equal(true) }) @@ -175,7 +163,7 @@ describe('ClsiManager', function() { status: (this.status = 'failure') } }) - return this.ClsiManager.sendRequest( + this.ClsiManager.sendRequest( this.project_id, this.user_id, {}, @@ -184,7 +172,7 @@ describe('ClsiManager', function() { }) it('should call the callback with a failure status', function() { - return this.callback.calledWith(null, this.status).should.equal(true) + this.callback.calledWith(null, this.status).should.equal(true) }) }) @@ -197,7 +185,7 @@ describe('ClsiManager', function() { this.ClsiManager.sendRequestOnce .withArgs(this.project_id, this.user_id, {}) .callsArgWith(3, null, 'conflict') - return this.ClsiManager.sendRequest( + this.ClsiManager.sendRequest( this.project_id, this.user_id, {}, @@ -206,23 +194,23 @@ describe('ClsiManager', function() { }) it('should call the sendRequestOnce method twice', function() { - return this.ClsiManager.sendRequestOnce.calledTwice.should.equal(true) + this.ClsiManager.sendRequestOnce.calledTwice.should.equal(true) }) it('should call the sendRequestOnce method with syncType:full', function() { - return this.ClsiManager.sendRequestOnce + this.ClsiManager.sendRequestOnce .calledWith(this.project_id, this.user_id, { syncType: 'full' }) .should.equal(true) }) it('should call the sendRequestOnce method without syncType:full', function() { - return this.ClsiManager.sendRequestOnce + this.ClsiManager.sendRequestOnce .calledWith(this.project_id, this.user_id, {}) .should.equal(true) }) it('should call the callback with a success status', function() { - return this.callback.calledWith(null, this.status).should.equal(true) + this.callback.calledWith(null, this.status).should.equal(true) }) }) @@ -236,7 +224,7 @@ describe('ClsiManager', function() { status: (this.status = 'failure') } }) - return this.ClsiManager.sendRequest( + this.ClsiManager.sendRequest( this.project_id, this.user_id, {}, @@ -245,13 +233,11 @@ describe('ClsiManager', function() { }) it('should call the callback only once', function() { - return this.callback.calledOnce.should.equal(true) + this.callback.calledOnce.should.equal(true) }) it('should call the callback with an error', function() { - return this.callback - .calledWithExactly(new Error('failed')) - .should.equal(true) + this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error)) }) }) }) @@ -260,7 +246,7 @@ describe('ClsiManager', function() { beforeEach(function() { this.submission_id = 'submission-id' this.clsi_request = 'mock-request' - return this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') + this.ClsiCookieManager._getServerId.callsArgWith(1, null, 'clsi3') }) describe('with a successful compile', function() { @@ -288,7 +274,7 @@ describe('ClsiManager', function() { ] } }) - return this.ClsiManager.sendExternalRequest( + this.ClsiManager.sendExternalRequest( this.submission_id, this.clsi_request, { compileGroup: 'standard' }, @@ -297,7 +283,7 @@ describe('ClsiManager', function() { }) it('should send the request to the CLSI', function() { - return this.ClsiManager._postToClsi + this.ClsiManager._postToClsi .calledWith(this.submission_id, null, this.clsi_request, 'standard') .should.equal(true) }) @@ -317,7 +303,7 @@ describe('ClsiManager', function() { build: 1234 } ] - return this.callback + this.callback .calledWith(null, this.status, outputFiles) .should.equal(true) }) @@ -330,7 +316,7 @@ describe('ClsiManager', function() { status: (this.status = 'failure') } }) - return this.ClsiManager.sendExternalRequest( + this.ClsiManager.sendExternalRequest( this.submission_id, this.clsi_request, {}, @@ -339,7 +325,7 @@ describe('ClsiManager', function() { }) it('should call the callback with a failure status', function() { - return this.callback.calledWith(null, this.status).should.equal(true) + this.callback.calledWith(null, this.status).should.equal(true) }) }) @@ -353,7 +339,7 @@ describe('ClsiManager', function() { status: (this.status = 'failure') } }) - return this.ClsiManager.sendExternalRequest( + this.ClsiManager.sendExternalRequest( this.submission_id, this.clsi_request, {}, @@ -362,13 +348,11 @@ describe('ClsiManager', function() { }) it('should call the callback only once', function() { - return this.callback.calledOnce.should.equal(true) + this.callback.calledOnce.should.equal(true) }) it('should call the callback with an error', function() { - return this.callback - .calledWithExactly(new Error('failed')) - .should.equal(true) + this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error)) }) }) }) @@ -376,14 +360,12 @@ describe('ClsiManager', function() { describe('deleteAuxFiles', function() { beforeEach(function() { this.ClsiManager._makeRequest = sinon.stub().callsArg(2) - return (this.DocumentUpdaterHandler.clearProjectState = sinon - .stub() - .callsArg(1)) + this.DocumentUpdaterHandler.clearProjectState = sinon.stub().callsArg(1) }) describe('with the standard compileGroup', function() { beforeEach(function() { - return this.ClsiManager.deleteAuxFiles( + this.ClsiManager.deleteAuxFiles( this.project_id, this.user_id, { compileGroup: 'standard' }, @@ -392,7 +374,7 @@ describe('ClsiManager', function() { }) it('should call the delete method in the standard CLSI', function() { - return this.ClsiManager._makeRequest + this.ClsiManager._makeRequest .calledWith(this.project_id, { method: 'DELETE', url: `${this.settings.apis.clsi.url}/project/${ @@ -403,13 +385,13 @@ describe('ClsiManager', function() { }) it('should clear the project state from the docupdater', function() { - return this.DocumentUpdaterHandler.clearProjectState + this.DocumentUpdaterHandler.clearProjectState .calledWith(this.project_id) .should.equal(true) }) it('should call the callback', function() { - return this.callback.called.should.equal(true) + this.callback.called.should.equal(true) }) }) }) @@ -454,25 +436,28 @@ describe('ClsiManager', function() { this.ProjectGetter.getProject = sinon .stub() .callsArgWith(2, null, this.project) - return (this.DocumentUpdaterHandler.flushProjectToMongo = sinon + this.DocumentUpdaterHandler.flushProjectToMongo = sinon .stub() - .callsArgWith(1, null)) + .callsArgWith(1, null) }) describe('with a valid project', function() { beforeEach(function(done) { - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { timeout: 100 }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } this.request = request - return done() + done() } ) }) it('should get the project with the required fields', function() { - return this.ProjectGetter.getProject + this.ProjectGetter.getProject .calledWith(this.project_id, { compiler: 1, rootDoc_id: 1, @@ -483,25 +468,25 @@ describe('ClsiManager', function() { }) it('should flush the project to the database', function() { - return this.DocumentUpdaterHandler.flushProjectToMongo + this.DocumentUpdaterHandler.flushProjectToMongo .calledWith(this.project_id) .should.equal(true) }) it('should get all the docs', function() { - return this.ProjectEntityHandler.getAllDocs + this.ProjectEntityHandler.getAllDocs .calledWith(this.project_id) .should.equal(true) }) it('should get all the files', function() { - return this.ProjectEntityHandler.getAllFiles + this.ProjectEntityHandler.getAllFiles .calledWith(this.project_id) .should.equal(true) }) it('should build up the CLSI request', function() { - return expect(this.request).to.deep.equal({ + expect(this.request).to.deep.equal({ compile: { options: { compiler: this.compiler, @@ -552,18 +537,21 @@ describe('ClsiManager', function() { this.ProjectEntityHandler.getAllDocPathsFromProject = sinon .stub() .callsArgWith(1, null, { 'mock-doc-id-1': 'main.tex' }) - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { timeout: 100, incrementalCompilesEnabled: true }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } this.request = request - return done() + done() } ) }) it('should get the project with the required fields', function() { - return this.ProjectGetter.getProject + this.ProjectGetter.getProject .calledWith(this.project_id, { compiler: 1, rootDoc_id: 1, @@ -574,23 +562,23 @@ describe('ClsiManager', function() { }) it('should not explicitly flush the project to the database', function() { - return this.DocumentUpdaterHandler.flushProjectToMongo + this.DocumentUpdaterHandler.flushProjectToMongo .calledWith(this.project_id) .should.equal(false) }) it('should get only the live docs from the docupdater with a background flush in docupdater', function() { - return this.DocumentUpdaterHandler.getProjectDocsIfMatch + this.DocumentUpdaterHandler.getProjectDocsIfMatch .calledWith(this.project_id) .should.equal(true) }) it('should not get any of the files', function() { - return this.ProjectEntityHandler.getAllFiles.called.should.equal(false) + this.ProjectEntityHandler.getAllFiles.called.should.equal(false) }) it('should build up the CLSI request', function() { - return expect(this.request).to.deep.equal({ + expect(this.request).to.deep.equal({ compile: { options: { compiler: this.compiler, @@ -632,22 +620,25 @@ describe('ClsiManager', function() { 'mock-doc-id-1': 'main.tex', 'mock-doc-id-2': '/chapters/chapter1.tex' }) - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { timeout: 100, incrementalCompilesEnabled: true, rootDoc_id: 'mock-doc-id-2' }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } this.request = request - return done() + done() } ) }) it('should still change the root path', function() { - return this.request.compile.rootResourcePath.should.equal( + this.request.compile.rootResourcePath.should.equal( 'chapters/chapter1.tex' ) }) @@ -656,18 +647,21 @@ describe('ClsiManager', function() { describe('when root doc override is valid', function() { beforeEach(function(done) { - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { rootDoc_id: 'mock-doc-id-2' }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } this.request = request - return done() + done() } ) }) it('should change root path', function() { - return this.request.compile.rootResourcePath.should.equal( + this.request.compile.rootResourcePath.should.equal( 'chapters/chapter1.tex' ) }) @@ -675,55 +669,53 @@ describe('ClsiManager', function() { describe('when root doc override is invalid', function() { beforeEach(function(done) { - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { rootDoc_id: 'invalid-id' }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } this.request = request - return done() + done() } ) }) it('should fallback to default root doc', function() { - return this.request.compile.rootResourcePath.should.equal('main.tex') + this.request.compile.rootResourcePath.should.equal('main.tex') }) }) describe('when the project has an invalid compiler', function() { beforeEach(function(done) { this.project.compiler = 'context' - return this.ClsiManager._buildRequest( - this.project, - null, - (error, request) => { - this.request = request - return done() + this.ClsiManager._buildRequest(this.project, null, (err, request) => { + if (err != null) { + return done(err) } - ) + this.request = request + done() + }) }) it('should set the compiler to pdflatex', function() { - return this.request.compile.options.compiler.should.equal('pdflatex') + this.request.compile.options.compiler.should.equal('pdflatex') }) }) describe('when there is no valid root document', function() { beforeEach(function(done) { this.project.rootDoc_id = 'not-valid' - return this.ClsiManager._buildRequest( - this.project, - null, - (error, request) => { - this.error = error - this.request = request - return done() - } - ) + this.ClsiManager._buildRequest(this.project, null, (error, request) => { + this.error = error + this.request = request + done() + }) }) it('should set to main.tex', function() { - return this.request.compile.rootResourcePath.should.equal('main.tex') + this.request.compile.rootResourcePath.should.equal('main.tex') }) }) @@ -745,13 +737,11 @@ describe('ClsiManager', function() { this.ProjectEntityHandler.getAllDocs = sinon .stub() .callsArgWith(1, null, this.docs) - return this.ClsiManager._buildRequest(this.project, null, this.callback) + this.ClsiManager._buildRequest(this.project, null, this.callback) }) it('should report an error', function() { - return this.callback - .calledWith(new Error('no main file specified')) - .should.equal(true) + this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error)) }) }) @@ -768,30 +758,29 @@ describe('ClsiManager', function() { this.ProjectEntityHandler.getAllDocs = sinon .stub() .callsArgWith(1, null, this.docs) - return this.ClsiManager._buildRequest( - this.project, - null, - (error, request) => { - this.error = error - this.request = request - return done() - } - ) + this.ClsiManager._buildRequest(this.project, null, (error, request) => { + this.error = error + this.request = request + done() + }) }) it('should set io to the only file', function() { - return this.request.compile.rootResourcePath.should.equal('other.tex') + this.request.compile.rootResourcePath.should.equal('other.tex') }) }) describe('with the draft option', function() { it('should add the draft option into the request', function(done) { - return this.ClsiManager._buildRequest( + this.ClsiManager._buildRequest( this.project_id, { timeout: 100, draft: true }, - (error, request) => { + (err, request) => { + if (err != null) { + return done(err) + } request.compile.options.draft.should.equal(true) - return done() + done() } ) }) @@ -800,7 +789,7 @@ describe('ClsiManager', function() { describe('_postToClsi', function() { beforeEach(function() { - return (this.req = { mock: 'req' }) + this.req = { mock: 'req' } }) describe('successfully', function() { @@ -813,7 +802,7 @@ describe('ClsiManager', function() { { statusCode: 204 }, (this.body = { mock: 'foo' }) ) - return this.ClsiManager._postToClsi( + this.ClsiManager._postToClsi( this.project_id, this.user_id, this.req, @@ -826,7 +815,7 @@ describe('ClsiManager', function() { const url = `${this.settings.apis.clsi.url}/project/${ this.project_id }/user/${this.user_id}/compile` - return this.ClsiManager._makeRequest + this.ClsiManager._makeRequest .calledWith(this.project_id, { method: 'POST', url, @@ -836,7 +825,7 @@ describe('ClsiManager', function() { }) it('should call the callback with the body and no error', function() { - return this.callback.calledWith(null, this.body).should.equal(true) + this.callback.calledWith(null, this.body).should.equal(true) }) }) @@ -850,7 +839,7 @@ describe('ClsiManager', function() { { statusCode: 500 }, (this.body = { mock: 'foo' }) ) - return this.ClsiManager._postToClsi( + this.ClsiManager._postToClsi( this.project_id, this.user_id, this.req, @@ -859,13 +848,8 @@ describe('ClsiManager', function() { ) }) - it('should call the callback with the body and the error', function() { - return this.callback - .calledWith( - new Error('CLSI returned non-success code: 500'), - this.body - ) - .should.equal(true) + it('should call the callback with an error', function() { + this.callback.should.have.been.calledWith(sinon.match.instanceOf(Error)) }) }) }) @@ -880,18 +864,18 @@ describe('ClsiManager', function() { { statusCode: 200 }, (this.body = { mock: 'foo' }) ) - return (this.ClsiManager._buildRequest = sinon.stub().callsArgWith( + this.ClsiManager._buildRequest = sinon.stub().callsArgWith( 2, null, (this.req = { compile: { rootResourcePath: 'rootfile.text', options: {} } }) - )) + ) }) describe('with root file', function() { beforeEach(function() { - return this.ClsiManager.wordCount( + this.ClsiManager.wordCount( this.project_id, this.user_id, false, @@ -901,7 +885,7 @@ describe('ClsiManager', function() { }) it('should call wordCount with root file', function() { - return this.ClsiManager._makeRequest + this.ClsiManager._makeRequest .calledWith(this.project_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${ @@ -913,13 +897,13 @@ describe('ClsiManager', function() { }) it('should call the callback', function() { - return this.callback.called.should.equal(true) + this.callback.called.should.equal(true) }) }) describe('with param file', function() { beforeEach(function() { - return this.ClsiManager.wordCount( + this.ClsiManager.wordCount( this.project_id, this.user_id, 'main.tex', @@ -929,7 +913,7 @@ describe('ClsiManager', function() { }) it('should call wordCount with param file', function() { - return this.ClsiManager._makeRequest + this.ClsiManager._makeRequest .calledWith(this.project_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${ @@ -945,7 +929,7 @@ describe('ClsiManager', function() { beforeEach(function() { this.req.compile.options.imageName = this.image = 'example.com/mock/image' - return this.ClsiManager.wordCount( + this.ClsiManager.wordCount( this.project_id, this.user_id, 'main.tex', @@ -955,7 +939,7 @@ describe('ClsiManager', function() { }) it('should call wordCount with file and image', function() { - return this.ClsiManager._makeRequest + this.ClsiManager._makeRequest .calledWith(this.project_id, { method: 'GET', url: `http://clsi.example.com/project/${this.project_id}/user/${ @@ -972,28 +956,28 @@ describe('ClsiManager', function() { beforeEach(function() { this.response = { there: 'something' } this.request.callsArgWith(1, null, this.response) - return (this.opts = { + this.opts = { method: 'SOMETHIGN', url: 'http://a place on the web' - }) + } }) it('should process a request with a cookie jar', function(done) { - return this.ClsiManager._makeRequest(this.project_id, this.opts, () => { + this.ClsiManager._makeRequest(this.project_id, this.opts, () => { const args = this.request.args[0] args[0].method.should.equal(this.opts.method) args[0].url.should.equal(this.opts.url) args[0].jar.should.equal(this.jar) - return done() + done() }) }) it('should set the cookie again on response as it might have changed', function(done) { - return this.ClsiManager._makeRequest(this.project_id, this.opts, () => { + this.ClsiManager._makeRequest(this.project_id, this.opts, () => { this.ClsiCookieManager.setServerId .calledWith(this.project_id, this.response) .should.equal(true) - return done() + done() }) }) }) @@ -1003,13 +987,13 @@ describe('ClsiManager', function() { this.settings.apis.clsi_new = { url: 'https://compiles.somewhere.test' } this.response = { there: 'something' } this.request.callsArgWith(1, null, this.response) - return (this.opts = { + this.opts = { url: this.ClsiManager._getCompilerUrl(null, this.project_id) - }) + } }) it('should change the domain on the url', function(done) { - return this.ClsiManager._makeNewBackendRequest( + this.ClsiManager._makeNewBackendRequest( this.project_id, this.opts, () => { @@ -1017,20 +1001,20 @@ describe('ClsiManager', function() { args[0].url.should.equal( `https://compiles.somewhere.test/project/${this.project_id}` ) - return done() + done() } ) }) it('should not make a request if there is not clsi_new url', function(done) { this.settings.apis.clsi_new = undefined - return this.ClsiManager._makeNewBackendRequest( + this.ClsiManager._makeNewBackendRequest( this.project_id, this.opts, err => { expect(err).to.equal(undefined) this.request.callCount.should.equal(0) - return done() + done() } ) })