Wr/progress bar off by one (#385)

* chore: update progressbar total when server resopnse changes + UT

* chore: fix never ending test

Co-authored-by: Shane McLaughlin <[email protected]>
Co-authored-by: Cristian Dominguez <[email protected]>

src/formatters/deployProgressBarFormatter.ts

@@ -11,6 +11,7 @@ import { once } from '@salesforce/kit';
 import cli from 'cli-ux';
 import { ProgressBar } from '../sourceCommand';
 import { ProgressFormatter } from './progressFormatter';
+
 export class DeployProgressBarFormatter extends ProgressFormatter {
   protected progressBar?: ProgressBar;
   public constructor(logger: Logger, ux: UX) {
@@ -26,21 +27,32 @@ export class DeployProgressBarFormatter extends ProgressFormatter {
 
     deploy.onUpdate((data) => {
       // the numCompTot. isn't computed right away, wait to start until we know how many we have
+      const total = data.numberComponentsTotal + data.numberTestsTotal;
       if (data.numberComponentsTotal) {
-        startProgressBar(data.numberComponentsTotal + data.numberTestsTotal);
+        startProgressBar(total);
         this.progressBar.update(data.numberComponentsDeployed + data.numberTestsCompleted);
       }
 
       // the numTestsTot. isn't computed until validated as tests by the server, update the PB once we know
       if (data.numberTestsTotal && data.numberComponentsTotal) {
-        this.progressBar.setTotal(data.numberComponentsTotal + data.numberTestsTotal);
+        this.progressBar.setTotal(total);
       }
     });
 
     // any thing else should stop the progress bar
     deploy.onFinish((data) => {
       // the final tick of `onUpdate` is actually fired with `onFinish`
-      this.progressBar.update(data.response.numberComponentsDeployed + data.response.numberTestsCompleted);
+      const deployed = data.response.numberComponentsDeployed + data.response.numberTestsCompleted;
+
+      // in cases when deploying only an object's customfields, without the custom object (think MPD)
+      // the server initially returns the number of customfields (n) + 1 - but once the deploy has finished
+      // it calculates the correct number of fields deployed n, and so we are left with a progress bar at n/(n+1)
+      // so if the progress bar total is different from what was actually deployed, set the total to be accurate
+      if (this.progressBar.total !== deployed) {
+        this.progressBar.setTotal(deployed);
+      }
+
+      this.progressBar.update(deployed);
       this.progressBar.stop();
     });
 
@@ -54,7 +66,7 @@ export class DeployProgressBarFormatter extends ProgressFormatter {
     });
   }
 
-  // used to intialise the progress bar
+  // used to initialise the progress bar
   protected initProgressBar(): void {
     this.logger.debug('initializing progress bar');
     this.progressBar = cli.progress({

src/sourceCommand.ts

@@ -12,6 +12,8 @@ import { get, getBoolean, getString, Optional } from '@salesforce/ts-types';
 import cli from 'cli-ux';
 
 export type ProgressBar = {
+  value: number;
+  total: number;
   start: (num: number) => void;
   update: (num: number) => void;
   updateTotal: (num: number) => void;

test/commands/source/deploy.test.ts

@@ -434,14 +434,6 @@ describe('force:source:deploy', () => {
       ensureHookArgs();
       ensureProgressBar(0);
     });
-
-    it('should start the progress bar only once with data onUpdate');
-    it('should display the deploy ID only once onUpdate');
-    it('should update progress bar with data onUpdate');
-    it('should setTotal on the progress bar with data onUpdate');
-    it('should update and stop progress bar with data onFinish');
-    it('should stop progress bar onCancel');
-    it('should stop progress bar onError');
   });
 
   it('should return JSON format and not display for a synchronous deploy', async () => {

test/commands/source/progressBarFormatter.test.ts

@@ -0,0 +1,170 @@
+/*
+ * Copyright (c) 2020, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { EventEmitter } from 'events';
+import { Logger } from '@salesforce/core';
+import { UX } from '@salesforce/command';
+import { MetadataApiDeploy } from '@salesforce/source-deploy-retrieve';
+import { spyMethod } from '@salesforce/ts-sinon';
+import { assert, expect } from 'chai';
+import * as sinon from 'sinon';
+import { DeployProgressBarFormatter } from '../../../src/formatters/deployProgressBarFormatter';
+import { ProgressBar } from '../../../src/sourceCommand';
+
+describe('Progress Bar Events', () => {
+  const sandbox = sinon.createSandbox();
+  const username = '[email protected]';
+  const deploy = new MetadataApiDeploy({ usernameOrConnection: username, id: '123' });
+  const progressBarFormatter = new DeployProgressBarFormatter(new Logger('testing'), UX.prototype);
+  const initSpy = spyMethod(sandbox, progressBarFormatter, 'initProgressBar');
+  let bar: ProgressBar;
+  let events: EventEmitter;
+
+  const overrideEvent = (event: EventEmitter) => {
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore the deploy must be listening to the same EventEmitter emitting events
+    deploy.event = event;
+  };
+
+  const getProgressbar = () => {
+    // eslint-disable-next-line @typescript-eslint/ban-ts-comment
+    // @ts-ignore protected member access
+    return progressBarFormatter.progressBar;
+  };
+
+  afterEach(() => {
+    sandbox.restore();
+  });
+
+  beforeEach(() => {
+    events = new EventEmitter();
+    overrideEvent(events);
+    progressBarFormatter.progress(deploy);
+    bar = getProgressbar();
+  });
+
+  it('should start the progress bar only once with data onUpdate', () => {
+    events.emit('update', {
+      numberComponentsTotal: 5,
+      numberComponentsDeployed: 3,
+      numberTestsCompleted: 0,
+      numberTestsTotal: 0,
+    });
+    expect(initSpy.calledOnce).to.be.true;
+    expect(bar.value).to.equal(3);
+    expect(bar.total).to.equal(5);
+    events.emit('finish', {
+      response: {
+        numberComponentsTotal: 5,
+        numberComponentsDeployed: 5,
+        numberTestsCompleted: 0,
+        numberTestsTotal: 0,
+      },
+    });
+  });
+
+  it('should update progress bar with data onUpdate, update once tests are calculated, update onFinish', () => {
+    events.emit('update', {
+      numberComponentsTotal: 10,
+      numberComponentsDeployed: 3,
+      numberTestsCompleted: 0,
+      numberTestsTotal: 0,
+    });
+    expect(bar.total).to.equal(10);
+    expect(bar.value).to.equal(3);
+    // deploy done, tests running
+    events.emit('update', {
+      numberComponentsTotal: 10,
+      numberComponentsDeployed: 10,
+      numberTestsCompleted: 5,
+      numberTestsTotal: 10,
+    });
+    expect(bar.total).to.equal(20);
+    expect(bar.value).to.equal(15);
+    // all done
+    events.emit('finish', {
+      response: {
+        numberComponentsTotal: 10,
+        numberComponentsDeployed: 10,
+        numberTestsCompleted: 10,
+        numberTestsTotal: 10,
+      },
+    });
+    expect(bar.value).to.equal(20);
+    expect(initSpy.calledOnce).to.be.true;
+  });
+
+  it('should update progress bar when server returns different calculated value', () => {
+    events.emit('update', {
+      numberComponentsTotal: 20,
+      numberComponentsDeployed: 3,
+      numberTestsCompleted: 0,
+      numberTestsTotal: 0,
+    });
+    expect(bar.total).to.equal(20);
+    expect(bar.value).to.equal(3);
+    // deploy done, tests running
+    events.emit('update', {
+      numberComponentsTotal: 20,
+      numberComponentsDeployed: 10,
+      numberTestsCompleted: 5,
+      numberTestsTotal: 10,
+    });
+    expect(bar.total).to.equal(30);
+    expect(bar.value).to.equal(15);
+    // all done - notice 19 comps. deployed
+    events.emit('finish', {
+      response: {
+        numberComponentsTotal: 20,
+        numberComponentsDeployed: 19,
+        numberTestsCompleted: 10,
+        numberTestsTotal: 10,
+      },
+    });
+    expect(bar.value).to.equal(29);
+    expect(bar.total).to.equal(29);
+    expect(initSpy.calledOnce).to.be.true;
+  });
+
+  it('should stop progress bar onCancel', () => {
+    const stopSpy = spyMethod(sandbox, bar, 'stop');
+
+    events.emit('update', {
+      numberComponentsTotal: 10,
+      numberComponentsDeployed: 3,
+      numberTestsCompleted: 0,
+      numberTestsTotal: 0,
+    });
+    expect(bar.total).to.equal(10);
+    expect(bar.value).to.equal(3);
+
+    events.emit('cancel');
+    expect(stopSpy.calledOnce).to.be.true;
+  });
+
+  it('should stop progress bar onError', () => {
+    const stopSpy = spyMethod(sandbox, bar, 'stop');
+
+    events.emit('update', {
+      numberComponentsTotal: 10,
+      numberComponentsDeployed: 3,
+      numberTestsCompleted: 0,
+      numberTestsTotal: 0,
+    });
+
+    expect(bar.total).to.equal(10);
+    expect(bar.value).to.equal(3);
+
+    try {
+      events.emit('error', new Error('error on deploy'));
+      assert.fail('the above should throw an error');
+    } catch (e) {
+      const err = e as Error;
+      expect(err.message).to.equal('error on deploy');
+    }
+    expect(stopSpy.calledOnce).to.be.true;
+  });
+});