Addresses code reviews

Author: Adam-it

src/m365/spfx/commands/project/project-azuredevops-pipeline-add.spec.ts

@@ -18,7 +18,7 @@ describe(commands.PROJECT_AZUREDEVOPS_PIPELINE_ADD, () => {
   let log: any[];
   let logger: Logger;
   let commandInfo: CommandInfo;
-  const projectPath: string = path.resolve('/fake/path/to/test-project');
+  const projectPath: string = path.resolve('/test-project');
 
   before(() => {
     sinon.stub(telemetry, 'trackEvent').resolves();
@@ -183,7 +183,7 @@ describe(commands.PROJECT_AZUREDEVOPS_PIPELINE_ADD, () => {
       throw `Invalid path: ${fakePath}`;
     });
 
-    sinon.stub(command as any, 'getProjectVersion').returns('');
+    sinon.stub(command as any, 'getProjectVersion').returns(undefined);
 
     sinon.stub(fs, 'writeFileSync').throws(new Error('writeFileSync failed'));
 
@@ -250,36 +250,4 @@ describe(commands.PROJECT_AZUREDEVOPS_PIPELINE_ADD, () => {
     await assert.rejects(command.action(logger, { options: {} } as any),
       new CommandError('writeFileSync failed'));
   });
-
-  it('handles unexpected non-error value', async () => {
-    sinon.stub(command as any, 'getProjectRoot').returns(projectPath);
-
-    sinon.stub(fs, 'readFileSync').callsFake((filePath, options) => {
-      if (filePath.toString() === path.join(projectPath, 'package.json') && options === 'utf-8') {
-        return '{"name": "test"}';
-      }
-
-      throw `Invalid path: ${filePath}`;
-    });
-
-    sinon.stub(fs, 'existsSync').callsFake((fakePath) => {
-      if (fakePath.toString() === path.join(projectPath, '.azuredevops')) {
-        return true;
-      }
-      else if (fakePath.toString() === path.join(projectPath, '.azuredevops', 'pipelines')) {
-        return true;
-      }
-
-      throw `Invalid path: ${fakePath}`;
-    });
-
-    sinon.stub(command as any, 'getProjectVersion').returns('1.21.1');
-
-    sinon.stub(fs, 'writeFileSync').callsFake(() => {
-      throw 'string failure';
-    });
-
-    await assert.rejects(command.action(logger, { options: {} } as any),
-      new CommandError('string failure'));
-  });
 });

src/m365/spfx/commands/project/project-github-workflow-add.spec.ts

@@ -188,13 +188,7 @@ describe(commands.PROJECT_GITHUB_WORKFLOW_ADD, () => {
 
     sinon.stub(fs, 'writeFileSync').throws(new Error('writeFileSync failed'));
 
-    await assert.rejects(command.action(logger, { options: {} }),
-      (err: any) => {
-        if (err instanceof CommandError && err.message.includes('Unable to determine the version')) {
-          return true;
-        }
-        return false;
-      });
+    await assert.rejects(command.action(logger, { options: {} }), new CommandError('Unable to determine the version of the current SharePoint Framework project. Could not find the correct version based on the version property in the .yo-rc.json file.'));
 
   });
 
@@ -224,13 +218,7 @@ describe(commands.PROJECT_GITHUB_WORKFLOW_ADD, () => {
 
     sinon.stub(fs, 'writeFileSync').throws(new Error('writeFileSync failed'));
 
-    await assert.rejects(command.action(logger, { options: {} }),
-      (err: any) => {
-        if (err instanceof CommandError && err.message.includes('Could not find Node version')) {
-          return true;
-        }
-        return false;
-      });
+    await assert.rejects(command.action(logger, { options: {} }), new CommandError("Could not find Node version for version '99.99.99' of SharePoint Framework."));
   });
 
   it('handles unexpected error', async () => {
@@ -257,43 +245,11 @@ describe(commands.PROJECT_GITHUB_WORKFLOW_ADD, () => {
 
     sinon.stub(command as any, 'getProjectVersion').returns('1.21.1');
 
-    sinon.stub(fs, 'writeFileSync').callsFake(() => {
-      throw new Error('writeFileSync failed');
-    });
+    sinon.stub(fs, 'writeFileSync').throws(
+      new Error('writeFileSync failed')
+    );
 
     await assert.rejects(command.action(logger, { options: {} } as any),
       new CommandError('writeFileSync failed'));
   });
-
-  it('handles unexpected non-error value', async () => {
-    sinon.stub(command as any, 'getProjectRoot').returns(projectPath);
-
-    sinon.stub(fs, 'readFileSync').callsFake((filePath, options) => {
-      if (filePath.toString() === path.join(projectPath, 'package.json') && options === 'utf-8') {
-        return '{"name": "test"}';
-      }
-
-      throw `Invalid path: ${filePath}`;
-    });
-
-    sinon.stub(fs, 'existsSync').callsFake((fakePath) => {
-      if (fakePath.toString() === path.join(projectPath, '.github')) {
-        return true;
-      }
-      else if (fakePath.toString() === path.join(projectPath, '.github', 'workflows')) {
-        return true;
-      }
-
-      throw `Invalid path: ${fakePath}`;
-    });
-
-    sinon.stub(command as any, 'getProjectVersion').returns('1.21.1');
-
-    sinon.stub(fs, 'writeFileSync').callsFake(() => {
-      throw 'string failure';
-    });
-
-    await assert.rejects(command.action(logger, { options: {} } as any),
-      new CommandError('string failure'));
-  });
 });

src/utils/spfx.spec.ts

@@ -45,7 +45,12 @@ describe('utils/spfx', () => {
 
   it('returns correct Node version when only minor version differ', () => {
     const version = spfx.getHighestNodeVersion('8.1 || 8.2');
-    assert.strictEqual(version, '8.x');
+    assert.strictEqual(version, '8.2.x');
+  });
+
+  it('returns correct Node version when only minor version differ - reverse case', () => {
+    const version = spfx.getHighestNodeVersion('8.2 || 8.1');
+    assert.strictEqual(version, '8.2.x');
   });
 
   it('returns highest major for disjoint ranges', () => {
@@ -75,7 +80,7 @@ describe('utils/spfx', () => {
 
   it('returns correct version for exact version without operator', () => {
     const version = spfx.getHighestNodeVersion('16.13.0');
-    assert.strictEqual(version, '16.x');
+    assert.strictEqual(version, '16.13.0');
   });
 
   it('returns highest version when mixing < and <= operators', () => {
@@ -90,8 +95,4 @@ describe('utils/spfx', () => {
   it('throws when range cannot be normalized', () => {
     assert.throws(() => spfx.getHighestNodeVersion('invalid-range'), new Error("Unable to resolve the highest Node version for range 'invalid-range'."));
   });
-
-  it('throws when no valid ranges found', () => {
-    assert.throws(() => spfx.getHighestNodeVersion('invalid-string'), new Error("Unable to resolve the highest Node version for range 'invalid-string'."));
-  });
 });

src/utils/spfx.ts

@@ -1,4 +1,4 @@
-import { Range, validRange } from 'semver';
+import { Range, coerce, gt, validRange } from 'semver';
 import { Project } from '../m365/spfx/commands/project/project-model/index.js';
 
 export const spfx = {
@@ -30,6 +30,21 @@ export const spfx = {
     let highestMajor: number | null = null;
     let exactVersion: string | null = null;
 
+    const simpleVersionPattern = /^\d+(\.\d+(\.\d+)?)?$/;
+    if (ranges.every(r => simpleVersionPattern.test(r))) {
+      const highest = ranges.reduce((best, curr) =>
+        gt(coerce(curr)!, coerce(best)!) ? curr : best
+      );
+      const parts = highest.split('.');
+      if (parts.length >= 3) {
+        return highest;
+      }
+
+      if (parts.length === 2) {
+        return `${highest}.x`;
+      }
+    }
+
     for (const rangeString of ranges) {
       const normalized = validRange(rangeString);
       if (!normalized) {
@@ -54,8 +69,8 @@ export const spfx = {
               exactVersion = comparator.semver.version;
             }
           }
-          else if (comparator.operator === '>=' || comparator.operator === '>' || comparator.operator === '') {
-            // For lower bounds or exact versions, use the major version
+          else if (comparator.operator === '>=' || comparator.operator === '>') {
+            // For lower bounds use the major version
             maxMajor = Math.max(maxMajor, comparator.semver.major);
           }
         }