Support different with and without hybrid benefits

parser/src/azurePortalExtensions.ts

@@ -1,5 +1,5 @@
 import * as puppeteer from 'puppeteer';
-import { PartialVmPricing, VmPricing } from "./vmPricing";
+import { addUnavailableVms, discardDuplicateVms, PartialVmPricing, VmPricing } from "./vmPricing";
 import { isSelectedSelect, setSelect, isSelectedCheckbox } from './puppeteerExtensions';
 
 export class AzurePortal {
@@ -67,72 +67,30 @@ export class AzurePortal {
     const operatingSystemWithHybridBenefit = ['windows', 'ml-server-windows', 'sharepoint', 'sql-server-enterprise', 'sql-server-standard', 'sql-server-web']
     const hasHybridBenefit = operatingSystemWithHybridBenefit.includes(operatingSystem);
 
-    const savingsPlanPricing = await this.p.evaluate(() => getPricing());
+    const savingsPlanPricing = await this.getUniquePricing();
     let savingsPlanWithHybridBenefits: PartialVmPricing[];
     let savingsPlansWithoutHybridBenefits: PartialVmPricing[];
 
     if (hasHybridBenefit) {
       savingsPlanWithHybridBenefits = savingsPlanPricing;
       await this.hideAzureHybridBenefit();
-      savingsPlansWithoutHybridBenefits = await this.p.evaluate(() => getPricing());
-
-      if (savingsPlanWithHybridBenefits.length !== savingsPlansWithoutHybridBenefits.length) {
-        const instancesWithHybridBenefits = savingsPlanWithHybridBenefits.map(v => v.instance);
-        const instancesWithoutHybridBenefits = savingsPlansWithoutHybridBenefits.map(v => v.instance);
-        const additionalWith = instancesWithHybridBenefits.filter(v => !instancesWithoutHybridBenefits.includes(v));
-        const additionalWithout = instancesWithoutHybridBenefits.filter(v => !instancesWithHybridBenefits.includes(v));
-
-        if (additionalWith.length > 0)
-        {
-          throw `Unexpected additional savings plan instance(s) with hybrid benefits: ${JSON.stringify(additionalWith)}. We only support additional without hybrid benefits instances.`;
-        }
-
-        additionalWithout.forEach(without => {
-          var offset = savingsPlansWithoutHybridBenefits.findIndex(v => v.instance == without);
-          var unavailableVm = <PartialVmPricing> {
-            instance: without,
-            vCpu: savingsPlansWithoutHybridBenefits[offset].vCpu,
-            ram: savingsPlansWithoutHybridBenefits[offset].ram
-          };
-          savingsPlanWithHybridBenefits.splice(offset, 0, unavailableVm);
-        });
-      }
+      savingsPlansWithoutHybridBenefits = await this.getUniquePricing();
+      addUnavailableVms(savingsPlanWithHybridBenefits, savingsPlansWithoutHybridBenefits);
     } else {
       savingsPlansWithoutHybridBenefits = savingsPlanPricing;
     }
 
     await this.selectReservedInstances();
 
-    const reservedPricing = await this.p.evaluate(() => getPricing());
+    const reservedPricing = await this.getUniquePricing();
     let reservedWithHybridBenefits: PartialVmPricing[];
     let reservedWithoutHybridBenefits: PartialVmPricing[];
 
     if (hasHybridBenefit) {
       reservedWithoutHybridBenefits = reservedPricing;
       await this.showAzureHybridBenefit();
-      reservedWithHybridBenefits = await this.p.evaluate(() => getPricing());
-
-      if (reservedWithoutHybridBenefits.length !== reservedWithHybridBenefits.length) {
-        const instancesWithHybridBenefits = reservedWithHybridBenefits.map(v => v.instance);
-        const instancesWithoutHybridBenefits = reservedWithoutHybridBenefits.map(v => v.instance);
-        const additionalWith = instancesWithHybridBenefits.filter(v => !instancesWithoutHybridBenefits.includes(v));
-        const additionalWithout = instancesWithoutHybridBenefits.filter(v => !instancesWithHybridBenefits.includes(v));
-
-        if (additionalWith.length > 0)
-        {
-          throw `Unexpected additional reserved instance(s) with hybrid benefits: ${JSON.stringify(additionalWith)}. We only support additional without hybrid benefits instances.`;
-        }
-
-        additionalWithout.forEach(without => {
-          var offset = reservedWithoutHybridBenefits.findIndex(v => v.instance == without);
-          var unavailableVm = <PartialVmPricing> {
-            instance: without,
-            vCpu: reservedWithoutHybridBenefits[offset].vCpu,
-            ram: reservedWithoutHybridBenefits[offset].ram
-          };
-          reservedWithHybridBenefits.splice(offset, 0, unavailableVm);
-        });
-      }
+      reservedWithHybridBenefits = await this.getUniquePricing();
+      addUnavailableVms(reservedWithHybridBenefits, reservedWithoutHybridBenefits);
     } else {
       reservedWithoutHybridBenefits = reservedPricing;
     }
@@ -266,6 +224,12 @@ export class AzurePortal {
       { timeout: 3000 }
     );
   }
+
+  private async getUniquePricing(): Promise<PartialVmPricing[]> {
+    let pricing = await this.p.evaluate(() => getPricing());
+    discardDuplicateVms(pricing);
+    return pricing;
+  }
 }
 
 export function getPrice(tr: HTMLTableRowElement, columnSelector: string): number {

parser/src/vmPricing.test.ts

@@ -0,0 +1,115 @@
+import { addUnavailableVms, discardDuplicateVms, PartialVmPricing } from "./vmPricing";
+
+function getVm(name: string, vCpu: number, ram: number): PartialVmPricing {
+  return <PartialVmPricing>{
+    instance: name,
+    vCpu: vCpu,
+    ram: ram
+  };
+}
+
+describe('Add unavailable VMs', () => {
+  describe('Given same VMs with and without hybrid benefits', () => {
+    test('Then VMs are unchanged', () => {
+      let withHybridBenefits = [getVm('A', 1, 1)];
+      let withoutHybridBenefits = [getVm('A', 1, 1)];
+
+      addUnavailableVms(withHybridBenefits, withoutHybridBenefits);
+
+      const expected = [getVm('A', 1, 1)];
+      expect(withHybridBenefits).toEqual(expected);
+      expect(withoutHybridBenefits).toEqual(expected);
+    });
+  });
+
+  describe('Given VM not present in without hybrid benefits list', () => {
+    test('Then VM added as unavailable to instances without hybrid benefits', () => {
+      let withHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+      let withoutHybridBenefits = [getVm('A', 1, 1), getVm('C', 3, 3)];
+
+      addUnavailableVms(withHybridBenefits, withoutHybridBenefits);
+
+      const expectedWithoutHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+      expect(withoutHybridBenefits).toEqual(expectedWithoutHybridBenefits);
+    });
+  });
+
+  describe('Given VM not present in with hybrid benefits list', () => {
+    test('Then VM added as unavailable to instances with hybrid benefits', () => {
+      let withHybridBenefits = [getVm('A', 1, 1), getVm('C', 3, 3)];
+      let withoutHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+
+      addUnavailableVms(withHybridBenefits, withoutHybridBenefits);
+
+      const expectedWithHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+      expect(withHybridBenefits).toEqual(expectedWithHybridBenefits);
+    });
+  });
+
+  describe('Given different VMs present in with and without hybrid benefits lists', () => {
+    test('Then VMs added as unavailable to both with and without hybrid benefits lists', () => {
+      let withHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('D', 4, 4)];
+      let withoutHybridBenefits = [getVm('A', 1, 1), getVm('C', 3, 3), getVm('D', 4, 4)];
+
+      addUnavailableVms(withHybridBenefits, withoutHybridBenefits);
+
+      const expected = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3), getVm('D', 4, 4)];
+      expect(withHybridBenefits).toEqual(expected);
+      expect(withoutHybridBenefits).toEqual(expected);
+    });
+  });
+
+  describe('Given multiple VMs in a row not present in without hybrid benefits list', () => {
+    test('Then VMs added as unavailable to without hybrid benefits list', () => {
+      let withHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3), getVm('D', 4, 4), getVm('E', 5, 5), getVm('F', 6, 6), getVm('G', 7, 7)];
+      let withoutHybridBenefits = [getVm('A', 1, 1), getVm('D', 4, 4), getVm('G', 7, 7)];
+
+      addUnavailableVms(withHybridBenefits, withoutHybridBenefits);
+
+      const expected = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3), getVm('D', 4, 4), getVm('E', 5, 5), getVm('F', 6, 6), getVm('G', 7, 7)];
+      expect(withoutHybridBenefits).toEqual(expected);
+    });
+  });
+
+  describe('Given duplicate with hybrid benefits', () => {
+    test('Then throw', () => {
+      let withHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('A', 1, 1)];
+      let withoutHybridBenefits = [getVm('A', 1, 1)];
+
+      expect(() => addUnavailableVms(withHybridBenefits, withoutHybridBenefits)).toThrow();
+    });
+  });
+
+  describe('Given duplicate without hybrid benefits', () => {
+    test('Then throw', () => {
+      let withHybridBenefits = [getVm('A', 1, 1)];
+      let withoutHybridBenefits = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('A', 1, 1)];
+
+      expect(() => addUnavailableVms(withHybridBenefits, withoutHybridBenefits)).toThrow();
+    });
+  });
+});
+
+describe('Discard duplicate VMs', () => {
+  describe('Given no duplicate', () => {
+    test('Then unmodified list', () => {
+      let vms = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+
+      discardDuplicateVms(vms);
+
+      const expected = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3)];
+      expect(vms).toEqual(expected);
+    });
+  });
+
+  describe('Given duplicates', () => {
+    test('Then duplicate instances are removed', () => {
+      let vms = [getVm('A', 1, 1), getVm('B', 2, 2), getVm('C', 3, 3), getVm('D', 4, 4), getVm('D', 5, 5), getVm('B', 6, 6), getVm('E', 7, 7), getVm('B', 8, 8)];
+
+      discardDuplicateVms(vms);
+
+      const expected = [getVm('A', 1, 1), getVm('C', 3, 3), getVm('E', 7, 7)];
+      expect(vms).toEqual(expected);
+    });
+  });
+});

parser/src/vmPricing.ts

@@ -25,3 +25,74 @@ export interface PartialVmPricing {
   threeYear: number;
   spot: number;
 }
+
+function addUnavailable(additionalInstances: string[], additional: PartialVmPricing[], toFill: PartialVmPricing[]): void {
+  additionalInstances.forEach(additionalInstance => {
+    var offset = additional.findIndex(v => v.instance == additionalInstance);
+    var unavailableVm = <PartialVmPricing> {
+      instance: additionalInstance,
+      vCpu: additional[offset].vCpu,
+      ram: additional[offset].ram
+    };
+    toFill.splice(offset, 0, unavailableVm);
+  });
+}
+
+function validateUnique(instances: string[]): void {
+  const uniqueWith = [...new Set(instances)];
+
+  if (instances.length !== uniqueWith.length) {
+    let duplicates: string[] = [];
+    instances.forEach((i, o) => {if (instances.indexOf(i) !== o) { duplicates.push(i); }});
+    throw `Instances contain duplicates ${duplicates}.`;
+  }
+}
+
+/**
+ * Ensure that if a VM is present in one of the arrays and not the other it will be added so that both arrays
+ * have the same VMs in the same order.
+ * @param withHybridBenefits the instances available with hybrid benefits. The array will be modified if the
+ * other one has additional instances.
+ * @param withoutHybridBenefits the instances available without hybrid benefits. The array will be modified
+ * if the other one has additional instances.
+ */
+export function addUnavailableVms(withHybridBenefits: PartialVmPricing[], withoutHybridBenefits: PartialVmPricing[]): void {
+  const instancesWithHybridBenefits = withHybridBenefits.map(v => v.instance);
+  validateUnique(instancesWithHybridBenefits);
+  const instancesWithoutHybridBenefits = withoutHybridBenefits.map(v => v.instance);
+  validateUnique(instancesWithoutHybridBenefits);
+  const additionalWith = instancesWithHybridBenefits.filter(v => !instancesWithoutHybridBenefits.includes(v));
+  const additionalWithout = instancesWithoutHybridBenefits.filter(v => !instancesWithHybridBenefits.includes(v));
+  addUnavailable(additionalWith, withHybridBenefits, withoutHybridBenefits);
+  addUnavailable(additionalWithout, withoutHybridBenefits, withHybridBenefits);
+}
+
+/**
+ * Remove duplicate instance names. Unfortunately the pricing sometimes contain duplicate VMs. When
+ * that's the case, only the name is duplicated, the specs and prices are correct. The easiest fix is
+ * to remove all instances, e.g. if VM 'A' appears twice, I remove both instances.
+ * @param vms The list of VMs we crawled from the pricing page. The array will be modified in-place.
+ */
+export function discardDuplicateVms(vms: PartialVmPricing[]): void {
+  const uniqueNames = new Map<string, number>();
+  const offsetsToRemove = new Set<number>();
+
+  for (let o = 0; o < vms.length; o++) {
+    const name = vms[o].instance;
+
+    if (uniqueNames.has(name)) {
+      offsetsToRemove.add(uniqueNames.get(name));
+      offsetsToRemove.add(o);
+    } else {
+      uniqueNames.set(name, o);
+    }
+  }
+
+  let deletedElementCount = 0;
+  // We need to sort the offset to remove because a duplicate name could appear before and after
+  // another duplicate, e.g. ['A, 'B', 'B', 'A'].
+  for (const offsetToRemove of Array.from(offsetsToRemove.keys()).sort()) {
+    vms.splice(offsetToRemove - deletedElementCount, 1);
+    deletedElementCount++;
+  }
+}