From 5d6d9488e46ba38f361426bbc234aadcb79723ea Mon Sep 17 00:00:00 2001 From: Kevin Cunnane Date: Mon, 17 Jul 2017 11:36:49 -0700 Subject: [PATCH] Fix #939 Allow messages to display run time for each batch when there are multiple batches (#943) * Fix #939 Allow messages to display run time for each batch when there are multiple batches - Added this feature behind a config flag - Some time parsing fixes to support this --- .../enu/constants/localizedConstants.enu.xlf | 3 ++ package.json | 5 ++++ src/constants/constants.ts | 1 + src/controllers/queryRunner.ts | 25 ++++++++++++++-- src/models/utils.ts | 10 +++++-- test/queryRunner.test.ts | 30 +++++++++++++++---- test/utils.test.ts | 3 +- 7 files changed, 67 insertions(+), 10 deletions(-) diff --git a/localization/xliff/enu/constants/localizedConstants.enu.xlf b/localization/xliff/enu/constants/localizedConstants.enu.xlf index d26085c8d6..949e8bfcd1 100644 --- a/localization/xliff/enu/constants/localizedConstants.enu.xlf +++ b/localization/xliff/enu/constants/localizedConstants.enu.xlf @@ -287,6 +287,9 @@ Are you sure you want to disconnect? + + Batch execution time: {0} + A SQL editor must have focus before executing this command diff --git a/package.json b/package.json index f4f64f6f63..4dc5d7b8d4 100644 --- a/package.json +++ b/package.json @@ -468,6 +468,11 @@ "description": "[Optional] Configuration options for copying multi-line results from the Results View", "default": true }, + "mssql.showBatchTime": { + "type": "boolean", + "description": "[Optional] Should execution time be shown for individual batches", + "default": false + }, "mssql.splitPaneSelection": { "type": "string", "description": "[Optional] Configuration options for which column new result panes should open in", diff --git a/src/constants/constants.ts b/src/constants/constants.ts index 7ccdc6bb47..5acd98f883 100644 --- a/src/constants/constants.ts +++ b/src/constants/constants.ts @@ -63,6 +63,7 @@ export const configRecentConnections = 'recentConnections'; export const configMaxRecentConnections = 'maxRecentConnections'; export const configCopyRemoveNewLine = 'copyRemoveNewLine'; export const configSplitPaneSelection = 'splitPaneSelection'; +export const configShowBatchTime = 'showBatchTime'; export const extConfigResultKeys = ['shortcuts', 'messagesDefaultOpen']; export const sqlToolsServiceInstallDirConfigKey = 'installDir'; export const sqlToolsServiceExecutableFilesConfigKey = 'executableFiles'; diff --git a/src/controllers/queryRunner.ts b/src/controllers/queryRunner.ts index 2f825e5597..3fc25bb02b 100644 --- a/src/controllers/queryRunner.ts +++ b/src/controllers/queryRunner.ts @@ -14,7 +14,7 @@ import { BatchSummary, QueryExecuteParams, QueryExecuteRequest, QueryExecuteBatchNotificationParams } from '../models/contracts/queryExecute'; import { QueryDisposeParams, QueryDisposeRequest } from '../models/contracts/queryDispose'; import { QueryCancelParams, QueryCancelResult, QueryCancelRequest } from '../models/contracts/queryCancel'; -import { ISlickRange, ISelectionData } from '../models/interfaces'; +import { ISlickRange, ISelectionData, IResultMessage } from '../models/interfaces'; import Constants = require('../constants/constants'); import LocalizedConstants = require('../constants/localizedConstants'); import * as Utils from './../models/utils'; @@ -213,7 +213,12 @@ export default class QueryRunner { // Store the batch again to get the rest of the data this._batchSets[batch.id] = batch; - this._totalElapsedMilliseconds += (Utils.parseTimeString(batch.executionElapsed) || 0); + let executionTime = (Utils.parseTimeString(batch.executionElapsed) || 0); + this._totalElapsedMilliseconds += executionTime; + if (executionTime > 0) { + // send a time message in the format used for query complete + this.sendBatchTimeMessage(batch.id, Utils.parseNumAsTimeString(executionTime)); + } this.eventEmitter.emit('batchComplete', batch); } @@ -364,6 +369,22 @@ export default class QueryRunner { return outputString; } + private sendBatchTimeMessage(batchId: number, executionTime: string): void { + // get config copyRemoveNewLine option from vscode config + let config = this._vscodeWrapper.getConfiguration(Constants.extensionConfigSectionName); + let showBatchTime: boolean = config[Constants.configShowBatchTime]; + if (showBatchTime) { + let message: IResultMessage = { + batchId: batchId, + message: Utils.formatString(LocalizedConstants.elapsedBatchTime, executionTime), + time: undefined, + isError: false + }; + // Send the message to the results pane + this.eventEmitter.emit('message', message); + } + } + /** * Sets a selection range in the editor for this query * @param selection The selection range to select diff --git a/src/models/utils.ts b/src/models/utils.ts index e8937e3eed..491285ffc5 100644 --- a/src/models/utils.ts +++ b/src/models/utils.ts @@ -310,11 +310,17 @@ export function parseTimeString(value: string): number | boolean { } let tempVal = value.split('.'); - if (tempVal.length !== 2) { + if (tempVal.length === 1) { + // Ideally would handle more cleanly than this but for now handle case where ms not set + tempVal = [tempVal[0], '0']; + } else if (tempVal.length !== 2) { return false; } - let ms = parseInt(tempVal[1].substring(0, 3), 10); + let msString = tempVal[1]; + let msStringEnd = msString.length < 3 ? msString.length : 3; + let ms = parseInt(tempVal[1].substring(0, msStringEnd), 10); + tempVal = tempVal[0].split(':'); if (tempVal.length !== 3) { diff --git a/test/queryRunner.test.ts b/test/queryRunner.test.ts index b333511d83..185d890a6e 100644 --- a/test/queryRunner.test.ts +++ b/test/queryRunner.test.ts @@ -4,6 +4,7 @@ import { EventEmitter } from 'events'; import QueryRunner from './../src/controllers/queryRunner'; import { QueryNotificationHandler } from './../src/controllers/queryNotificationHandler'; import { SqlOutputContentProvider } from './../src/models/sqlOutputContentProvider'; +import * as Utils from './../src/models/utils'; import SqlToolsServerClient from './../src/languageservice/serviceclient'; import { QueryExecuteParams, @@ -185,14 +186,21 @@ suite('Query Runner tests', () => { mockEventEmitter.verify(x => x.emit('batchStart', TypeMoq.It.isAny()), TypeMoq.Times.once()); }); - test('Notification - Batch Complete', () => { + function testBatchCompleteNotification(sendBatchTime: boolean): void { // Setup: Create a batch completion result + let configResult: {[key: string]: any} = {}; + configResult[Constants.configShowBatchTime] = sendBatchTime; + setupWorkspaceConfig(configResult); + + let dateNow = new Date(); + let fiveSecondsAgo = new Date(dateNow.getTime() - 5000); + let elapsedTimeString = Utils.parseNumAsTimeString(5000); let batchComplete: QueryExecuteBatchNotificationParams = { ownerUri: 'uri', batchSummary: { - executionElapsed: undefined, - executionEnd: new Date().toISOString(), - executionStart: new Date().toISOString(), + executionElapsed: elapsedTimeString, + executionEnd: dateNow.toISOString(), + executionStart: fiveSecondsAgo.toISOString(), hasError: false, id: 0, selection: {startLine: 0, endLine: 0, startColumn: 3, endColumn: 3}, @@ -220,13 +228,14 @@ suite('Query Runner tests', () => { }; let mockEventEmitter = TypeMoq.Mock.ofType(EventEmitter, TypeMoq.MockBehavior.Strict); mockEventEmitter.setup(x => x.emit('batchComplete', TypeMoq.It.isAny())); + mockEventEmitter.setup(x => x.emit('message', TypeMoq.It.isAny())); queryRunner.eventEmitter = mockEventEmitter.object; queryRunner.handleBatchComplete(batchComplete); // Then: It should the remainder of the information and emit a batch complete notification assert.equal(queryRunner.batchSets.length, 1); let storedBatch = queryRunner.batchSets[0]; - assert.equal(storedBatch.executionElapsed, undefined); + assert.equal(storedBatch.executionElapsed, elapsedTimeString); assert.equal(typeof(storedBatch.executionEnd), typeof(batchComplete.batchSummary.executionEnd)); assert.equal(typeof(storedBatch.executionStart), typeof(batchComplete.batchSummary.executionStart)); assert.equal(storedBatch.hasError, batchComplete.batchSummary.hasError); @@ -236,6 +245,17 @@ suite('Query Runner tests', () => { assert.equal(storedBatch.resultSetSummaries.length, 0); mockEventEmitter.verify(x => x.emit('batchComplete', TypeMoq.It.isAny()), TypeMoq.Times.once()); + let expectedMessageTimes = sendBatchTime ? TypeMoq.Times.once() : TypeMoq.Times.never(); + mockEventEmitter.verify(x => x.emit('message', TypeMoq.It.isAny()), expectedMessageTimes); + + } + + test('Notification - Batch Complete no message', () => { + testBatchCompleteNotification(false); + }); + + test('Notification - Batch Complete with message', () => { + testBatchCompleteNotification(true); }); test('Notification - ResultSet Complete w/no previous results', () => { diff --git a/test/utils.test.ts b/test/utils.test.ts index 0b565d5023..25a6b5023f 100644 --- a/test/utils.test.ts +++ b/test/utils.test.ts @@ -11,7 +11,6 @@ suite('Utility Tests - parseTimeString', () => { test('should return false if input does not have only 1 period', () => { expect(Utils.parseTimeString('32:13:23.12.1')).to.equal(false); - expect(Utils.parseTimeString('12:32:33')).to.equal(false); }); test('should return false if input does not have 2 :', () => { @@ -23,6 +22,8 @@ suite('Utility Tests - parseTimeString', () => { expect(Utils.parseTimeString('2:13:30.0')).to.equal(8010000); expect(Utils.parseTimeString('0:0:0.220')).to.equal(220); expect(Utils.parseTimeString('0:0:0.0')).to.equal(0); + // Allow time without milliseconds + expect(Utils.parseTimeString('2:13:30')).to.equal(8010000); }); });