Merge pull request #1788 from github/henrymercer/fix-feature-flag-usage

Fixes for new analysis summary and scaling reserved RAM feature flag usage

lib/codeql.js

@@ -467,10 +467,10 @@ async function getCodeQLForCmd(cmd, checkVersion) {
             else if (await util.codeQlVersionAbove(this, "2.12.4")) {
                 codeqlArgs.push("--no-sarif-include-diagnostics");
             }
-            if (await features.getValue(feature_flags_1.Feature.NewAnalysisSummaryEnabled, codeql)) {
+            if (await features.getValue(feature_flags_1.Feature.NewAnalysisSummaryEnabled, this)) {
                 codeqlArgs.push("--new-analysis-summary");
             }
-            else if (await util.codeQlVersionAbove(codeql, exports.CODEQL_VERSION_NEW_ANALYSIS_SUMMARY)) {
+            else if (await util.codeQlVersionAbove(this, exports.CODEQL_VERSION_NEW_ANALYSIS_SUMMARY)) {
                 codeqlArgs.push("--no-new-analysis-summary");
             }
             codeqlArgs.push(databasePath);

lib/codeql.test.js

@@ -668,6 +668,42 @@ const injectedConfigMacro = ava_1.default.macro({
     await codeqlObject.databaseInterpretResults("", [], "", "", "", "-v", "", stubConfig, (0, testing_utils_1.createFeatures)([]), (0, logging_1.getRunnerLogger)(true));
     t.false(runnerConstructorStub.firstCall.args[1].includes("--sarif-add-baseline-file-info"), "--sarif-add-baseline-file-info must be absent, but it is present");
 });
+const NEW_ANALYSIS_SUMMARY_TEST_CASES = [
+    {
+        featureEnabled: true,
+        codeqlVersion: "2.14.0",
+        flagPassed: true,
+        negativeFlagPassed: false,
+    },
+    {
+        featureEnabled: false,
+        codeqlVersion: "2.14.0",
+        flagPassed: false,
+        negativeFlagPassed: true,
+    },
+    {
+        featureEnabled: false,
+        codeqlVersion: "2.13.5",
+        flagPassed: false,
+        negativeFlagPassed: false,
+    },
+];
+for (const { featureEnabled, codeqlVersion, flagPassed, negativeFlagPassed, } of NEW_ANALYSIS_SUMMARY_TEST_CASES) {
+    (0, ava_1.default)(`database interpret-results passes ${flagPassed
+        ? "--new-analysis-summary"
+        : negativeFlagPassed
+            ? "--no-new-analysis-summary"
+            : "nothing"} for CodeQL CLI v${codeqlVersion} when the new analysis summary feature is ${featureEnabled ? "enabled" : "disabled"}`, async (t) => {
+        const runnerConstructorStub = stubToolRunnerConstructor();
+        const codeqlObject = await codeql.getCodeQLForTesting();
+        sinon.stub(codeqlObject, "getVersion").resolves(codeqlVersion);
+        // safeWhich throws because of the test CodeQL object.
+        sinon.stub(safeWhich, "safeWhich").resolves("");
+        await codeqlObject.databaseInterpretResults("", [], "", "", "", "-v", "", stubConfig, (0, testing_utils_1.createFeatures)(featureEnabled ? [feature_flags_1.Feature.NewAnalysisSummaryEnabled] : []), (0, logging_1.getRunnerLogger)(true));
+        t.is(runnerConstructorStub.firstCall.args[1].includes("--new-analysis-summary"), flagPassed, `--new-analysis-summary should${flagPassed ? "" : "n't"} be passed`);
+        t.is(runnerConstructorStub.firstCall.args[1].includes("--no-new-analysis-summary"), negativeFlagPassed, `--no-new-analysis-summary should${negativeFlagPassed ? "" : "n't"} be passed`);
+    });
+}
 function stubToolRunnerConstructor() {
     const runnerObjectStub = sinon.createStubInstance(toolrunner.ToolRunner);
     runnerObjectStub.exec.resolves(0);

lib/feature-flags.js

@@ -33,6 +33,11 @@ const defaults = __importStar(require("./defaults.json"));
 const util = __importStar(require("./util"));
 const DEFAULT_VERSION_FEATURE_FLAG_PREFIX = "default_codeql_version_";
 const DEFAULT_VERSION_FEATURE_FLAG_SUFFIX = "_enabled";
+/**
+ * Feature enablement as returned by the GitHub API endpoint.
+ *
+ * Each value of this enum should end with `_enabled`.
+ */
 var Feature;
 (function (Feature) {
     Feature["CliConfigFileEnabled"] = "cli_config_file_enabled";
@@ -42,7 +47,7 @@ var Feature;
     Feature["MlPoweredQueriesEnabled"] = "ml_powered_queries_enabled";
     Feature["NewAnalysisSummaryEnabled"] = "new_analysis_summary_enabled";
     Feature["QaTelemetryEnabled"] = "qa_telemetry_enabled";
-    Feature["ScalingReservedRam"] = "scaling_reserved_ram";
+    Feature["ScalingReservedRamEnabled"] = "scaling_reserved_ram_enabled";
     Feature["UploadFailedSarifEnabled"] = "upload_failed_sarif_enabled";
 })(Feature || (exports.Feature = Feature = {}));
 exports.featureConfig = {
@@ -76,7 +81,7 @@ exports.featureConfig = {
         minimumVersion: undefined,
         defaultValue: false,
     },
-    [Feature.ScalingReservedRam]: {
+    [Feature.ScalingReservedRamEnabled]: {
         envVar: "CODEQL_ACTION_SCALING_RESERVED_RAM",
         minimumVersion: undefined,
         defaultValue: false,

lib/feature-flags.test.js

@@ -309,6 +309,11 @@ for (const variant of [util_1.GitHubVariant.GHAE, util_1.GitHubVariant.GHES]) {
                 "Ignoring feature flag default_codeql_version_2_20_invalid_enabled as it does not specify a valid CodeQL version.") !== undefined);
     });
 });
+(0, ava_1.default)("feature flags should end with _enabled", async (t) => {
+    for (const feature of Object.values(feature_flags_1.Feature)) {
+        t.assert(feature.endsWith("_enabled"), `${feature} should end with '_enabled'`);
+    }
+});
 function assertAllFeaturesUndefinedInApi(t, loggedMessages) {
     for (const feature of Object.keys(feature_flags_1.featureConfig)) {
         t.assert(loggedMessages.find((v) => v.type === "debug" &&

lib/util.js

@@ -111,7 +111,7 @@ exports.withTmpDir = withTmpDir;
 async function getSystemReservedMemoryMegaBytes(totalMemoryMegaBytes, features) {
     // Windows needs more memory for OS processes.
     const fixedAmount = 1024 * (process.platform === "win32" ? 1.5 : 1);
-    if (await features.getValue(feature_flags_1.Feature.ScalingReservedRam)) {
+    if (await features.getValue(feature_flags_1.Feature.ScalingReservedRamEnabled)) {
         // Reserve an additional 2% of the total memory, since the amount used by
         // the kernel for page tables scales with the size of physical memory.
         const scaledAmount = 0.02 * totalMemoryMegaBytes;

lib/util.test.js

@@ -57,7 +57,7 @@ const util = __importStar(require("./util"));
         ["", true, `--ram=${expectedMemoryValueWithScaling}`],
     ];
     for (const [input, withScaling, expectedFlag] of tests) {
-        const features = (0, testing_utils_1.createFeatures)(withScaling ? [feature_flags_1.Feature.ScalingReservedRam] : []);
+        const features = (0, testing_utils_1.createFeatures)(withScaling ? [feature_flags_1.Feature.ScalingReservedRamEnabled] : []);
         const flag = await util.getMemoryFlag(input, features);
         t.deepEqual(flag, expectedFlag);
     }

src/codeql.test.ts

@@ -1061,6 +1061,78 @@ test("databaseInterpretResults() does not set --sarif-add-baseline-file-info for
   );
 });
 
+const NEW_ANALYSIS_SUMMARY_TEST_CASES = [
+  {
+    featureEnabled: true,
+    codeqlVersion: "2.14.0",
+    flagPassed: true,
+    negativeFlagPassed: false,
+  },
+  {
+    featureEnabled: false,
+    codeqlVersion: "2.14.0",
+    flagPassed: false,
+    negativeFlagPassed: true,
+  },
+  {
+    featureEnabled: false,
+    codeqlVersion: "2.13.5",
+    flagPassed: false,
+    negativeFlagPassed: false,
+  },
+];
+
+for (const {
+  featureEnabled,
+  codeqlVersion,
+  flagPassed,
+  negativeFlagPassed,
+} of NEW_ANALYSIS_SUMMARY_TEST_CASES) {
+  test(`database interpret-results passes ${
+    flagPassed
+      ? "--new-analysis-summary"
+      : negativeFlagPassed
+      ? "--no-new-analysis-summary"
+      : "nothing"
+  } for CodeQL CLI v${codeqlVersion} when the new analysis summary feature is ${
+    featureEnabled ? "enabled" : "disabled"
+  }`, async (t) => {
+    const runnerConstructorStub = stubToolRunnerConstructor();
+    const codeqlObject = await codeql.getCodeQLForTesting();
+    sinon.stub(codeqlObject, "getVersion").resolves(codeqlVersion);
+    // safeWhich throws because of the test CodeQL object.
+    sinon.stub(safeWhich, "safeWhich").resolves("");
+    await codeqlObject.databaseInterpretResults(
+      "",
+      [],
+      "",
+      "",
+      "",
+      "-v",
+      "",
+      stubConfig,
+      createFeatures(featureEnabled ? [Feature.NewAnalysisSummaryEnabled] : []),
+      getRunnerLogger(true)
+    );
+    t.is(
+      runnerConstructorStub.firstCall.args[1].includes(
+        "--new-analysis-summary"
+      ),
+      flagPassed,
+      `--new-analysis-summary should${flagPassed ? "" : "n't"} be passed`
+    );
+    t.is(
+      runnerConstructorStub.firstCall.args[1].includes(
+        "--no-new-analysis-summary"
+      ),
+      negativeFlagPassed,
+      `--no-new-analysis-summary should${
+        negativeFlagPassed ? "" : "n't"
+      } be passed`
+    );
+  });
+}
+
 export function stubToolRunnerConstructor(): sinon.SinonStub<
   any[],
   toolrunner.ToolRunner

src/codeql.ts

@@ -826,13 +826,10 @@ export async function getCodeQLForCmd(
       } else if (await util.codeQlVersionAbove(this, "2.12.4")) {
         codeqlArgs.push("--no-sarif-include-diagnostics");
       }
-      if (await features.getValue(Feature.NewAnalysisSummaryEnabled, codeql)) {
+      if (await features.getValue(Feature.NewAnalysisSummaryEnabled, this)) {
         codeqlArgs.push("--new-analysis-summary");
       } else if (
-        await util.codeQlVersionAbove(
+        await util.codeQlVersionAbove(this, CODEQL_VERSION_NEW_ANALYSIS_SUMMARY)
-          codeql,
-          CODEQL_VERSION_NEW_ANALYSIS_SUMMARY
-        )
       ) {
         codeqlArgs.push("--no-new-analysis-summary");
       }

src/feature-flags.test.ts

@@ -482,6 +482,15 @@ test("ignores invalid version numbers in default version feature flags", async (
   });
 });
 
+test("feature flags should end with _enabled", async (t) => {
+  for (const feature of Object.values(Feature)) {
+    t.assert(
+      feature.endsWith("_enabled"),
+      `${feature} should end with '_enabled'`
+    );
+  }
+});
+
 function assertAllFeaturesUndefinedInApi(
   t: ExecutionContext<unknown>,
   loggedMessages: LoggedMessage[]

src/feature-flags.ts

@@ -31,6 +31,11 @@ export interface FeatureEnablement {
   getValue(feature: Feature, codeql?: CodeQL): Promise<boolean>;
 }
 
+/**
+ * Feature enablement as returned by the GitHub API endpoint.
+ *
+ * Each value of this enum should end with `_enabled`.
+ */
 export enum Feature {
   CliConfigFileEnabled = "cli_config_file_enabled",
   DisableKotlinAnalysisEnabled = "disable_kotlin_analysis_enabled",
@@ -39,7 +44,7 @@ export enum Feature {
   MlPoweredQueriesEnabled = "ml_powered_queries_enabled",
   NewAnalysisSummaryEnabled = "new_analysis_summary_enabled",
   QaTelemetryEnabled = "qa_telemetry_enabled",
-  ScalingReservedRam = "scaling_reserved_ram",
+  ScalingReservedRamEnabled = "scaling_reserved_ram_enabled",
   UploadFailedSarifEnabled = "upload_failed_sarif_enabled",
 }
 
@@ -77,7 +82,7 @@ export const featureConfig: Record<
     minimumVersion: undefined,
     defaultValue: false,
   },
-  [Feature.ScalingReservedRam]: {
+  [Feature.ScalingReservedRamEnabled]: {
     envVar: "CODEQL_ACTION_SCALING_RESERVED_RAM",
     minimumVersion: undefined,
     defaultValue: false,

src/util.test.ts

@@ -48,7 +48,7 @@ test("getMemoryFlag() should return the correct --ram flag", async (t) => {
 
   for (const [input, withScaling, expectedFlag] of tests) {
     const features = createFeatures(
-      withScaling ? [Feature.ScalingReservedRam] : []
+      withScaling ? [Feature.ScalingReservedRamEnabled] : []
     );
     const flag = await util.getMemoryFlag(input, features);
     t.deepEqual(flag, expectedFlag);

src/util.ts

@@ -164,7 +164,7 @@ async function getSystemReservedMemoryMegaBytes(
   // Windows needs more memory for OS processes.
   const fixedAmount = 1024 * (process.platform === "win32" ? 1.5 : 1);
 
-  if (await features.getValue(Feature.ScalingReservedRam)) {
+  if (await features.getValue(Feature.ScalingReservedRamEnabled)) {
     // Reserve an additional 2% of the total memory, since the amount used by
     // the kernel for page tables scales with the size of physical memory.
     const scaledAmount = 0.02 * totalMemoryMegaBytes;