Skip to content

Commit faca00d

Browse files
sam-robsonsam-robson
committed
refactor: address review feedback on overlay fallback
1 parent 5d1c584 commit faca00d

5 files changed

Lines changed: 251 additions & 128 deletions

File tree

‎lib/init-action.js‎

Lines changed: 24 additions & 27 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/config-utils.test.ts‎

Lines changed: 54 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -2201,79 +2201,67 @@ test.serial(
22012201
},
22022202
);
22032203

2204-
test.serial(
2205-
"applyIncrementalAnalysisSettings: no-op when mode is not Overlay and diff-informed is unavailable",
2206-
(t) => {
2207-
const config = createTestConfig({});
2208-
config.overlayDatabaseMode = OverlayDatabaseMode.None;
2209-
const logger = getRunnerLogger(true);
2204+
test("applyIncrementalAnalysisSettings: no-op when mode is not Overlay and diff-informed is unavailable", (t) => {
2205+
const config = createTestConfig({});
2206+
config.overlayDatabaseMode = OverlayDatabaseMode.None;
2207+
const logger = getRunnerLogger(true);
2208+
2209+
configUtils.applyIncrementalAnalysisSettings(
2210+
config,
2211+
{ shouldRun: false, hasDiffRanges: false },
2212+
logger,
2213+
);
22102214

2211-
configUtils.applyIncrementalAnalysisSettings(
2212-
config,
2213-
{ shouldRun: false, isAvailable: false },
2214-
logger,
2215-
);
2215+
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2216+
t.deepEqual(config.extraQueryExclusions, []);
2217+
});
22162218

2217-
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2218-
t.deepEqual(config.extraQueryExclusions, []);
2219-
},
2220-
);
2219+
test("applyIncrementalAnalysisSettings: keeps overlay mode and adds exclusions when diff-informed analysis shouldn't run", (t) => {
2220+
const config = createTestConfig({});
2221+
config.overlayDatabaseMode = OverlayDatabaseMode.Overlay;
2222+
const logger = getRunnerLogger(true);
22212223

2222-
test.serial(
2223-
"applyIncrementalAnalysisSettings: keeps overlay mode and adds exclusions when diff-informed analysis is disabled",
2224-
(t) => {
2225-
const config = createTestConfig({});
2226-
config.overlayDatabaseMode = OverlayDatabaseMode.Overlay;
2227-
const logger = getRunnerLogger(true);
2228-
2229-
configUtils.applyIncrementalAnalysisSettings(
2230-
config,
2231-
{ shouldRun: false, isAvailable: false },
2232-
logger,
2233-
);
2224+
configUtils.applyIncrementalAnalysisSettings(
2225+
config,
2226+
{ shouldRun: false, hasDiffRanges: false },
2227+
logger,
2228+
);
22342229

2235-
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.Overlay);
2236-
t.deepEqual(config.extraQueryExclusions, [
2237-
{ exclude: { tags: "exclude-from-incremental" } },
2238-
]);
2239-
},
2240-
);
2230+
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.Overlay);
2231+
t.deepEqual(config.extraQueryExclusions, [
2232+
{ exclude: { tags: "exclude-from-incremental" } },
2233+
]);
2234+
});
22412235

2242-
test.serial(
2243-
"applyIncrementalAnalysisSettings: reverts to None without exclusions when diff-informed analysis is unavailable",
2244-
(t) => {
2245-
const config = createTestConfig({});
2246-
config.overlayDatabaseMode =
2247-
OverlayDatabaseMode.Overlay as OverlayDatabaseMode;
2248-
const logger = getRunnerLogger(true);
2236+
test("applyIncrementalAnalysisSettings: disables overlay analysis when diff-informed analysis is unavailable", (t) => {
2237+
const config = createTestConfig({
2238+
overlayDatabaseMode: OverlayDatabaseMode.Overlay,
2239+
});
2240+
const logger = getRunnerLogger(true);
22492241

2250-
configUtils.applyIncrementalAnalysisSettings(
2251-
config,
2252-
{ shouldRun: true, isAvailable: false },
2253-
logger,
2254-
);
2242+
configUtils.applyIncrementalAnalysisSettings(
2243+
config,
2244+
{ shouldRun: true, hasDiffRanges: false },
2245+
logger,
2246+
);
22552247

2256-
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2257-
t.deepEqual(config.extraQueryExclusions, []);
2258-
},
2259-
);
2248+
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2249+
t.deepEqual(config.extraQueryExclusions, []);
2250+
});
22602251

2261-
test.serial(
2262-
"applyIncrementalAnalysisSettings: adds exclusions for diff-informed-only runs",
2263-
(t) => {
2264-
const config = createTestConfig({});
2265-
config.overlayDatabaseMode = OverlayDatabaseMode.None;
2266-
const logger = getRunnerLogger(true);
2252+
test("applyIncrementalAnalysisSettings: adds exclusions for diff-informed-only runs", (t) => {
2253+
const config = createTestConfig({});
2254+
config.overlayDatabaseMode = OverlayDatabaseMode.None;
2255+
const logger = getRunnerLogger(true);
22672256

2268-
configUtils.applyIncrementalAnalysisSettings(
2269-
config,
2270-
{ shouldRun: true, isAvailable: true },
2271-
logger,
2272-
);
2257+
configUtils.applyIncrementalAnalysisSettings(
2258+
config,
2259+
{ shouldRun: true, hasDiffRanges: true },
2260+
logger,
2261+
);
22732262

2274-
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2275-
t.deepEqual(config.extraQueryExclusions, [
2276-
{ exclude: { tags: "exclude-from-incremental" } },
2277-
]);
2278-
},
2279-
);
2263+
t.is(config.overlayDatabaseMode, OverlayDatabaseMode.None);
2264+
t.deepEqual(config.extraQueryExclusions, [
2265+
{ exclude: { tags: "exclude-from-incremental" } },
2266+
]);
2267+
});

‎src/config-utils.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,8 +1085,14 @@ function hasQueryCustomisation(userConfig: UserConfig): boolean {
10851085
* If overlay mode was selected for a PR but diff-informed analysis should have
10861086
* run and could not be prepared, fall back to a full non-overlay analysis.
10871087
* Query exclusions for incremental-only queries are applied only when the final
1088-
* configuration still uses overlay analysis or diff-informed analysis is
1089-
* actually available.
1088+
* configuration still uses overlay analysis or the diff ranges are available.
1089+
*
1090+
* Note that `overlayDatabaseMode === Overlay` does not imply
1091+
* `diffInformedAnalysis.shouldRun`. Overlay mode is selected based on language
1092+
* and feature-flag state and can apply outside of pull-request contexts (e.g.
1093+
* on branch pushes that build up the overlay cache), whereas diff-informed
1094+
* analysis only runs for pull requests where we can compute a diff. Each
1095+
* combination is therefore handled explicitly.
10901096
*/
10911097
export function applyIncrementalAnalysisSettings(
10921098
config: Config,
@@ -1096,9 +1102,9 @@ export function applyIncrementalAnalysisSettings(
10961102
if (
10971103
config.overlayDatabaseMode === OverlayDatabaseMode.Overlay &&
10981104
diffInformedAnalysis.shouldRun &&
1099-
!diffInformedAnalysis.isAvailable
1105+
!diffInformedAnalysis.hasDiffRanges
11001106
) {
1101-
logger.warning(
1107+
logger.info(
11021108
"Diff-informed analysis is not available for this pull request. " +
11031109
`Reverting overlay database mode to ${OverlayDatabaseMode.None}.`,
11041110
);
@@ -1107,7 +1113,7 @@ export function applyIncrementalAnalysisSettings(
11071113

11081114
if (
11091115
config.overlayDatabaseMode === OverlayDatabaseMode.Overlay ||
1110-
diffInformedAnalysis.isAvailable
1116+
diffInformedAnalysis.hasDiffRanges
11111117
) {
11121118
config.extraQueryExclusions.push({
11131119
exclude: { tags: "exclude-from-incremental" },

‎src/diff-informed-analysis-utils.test.ts‎

Lines changed: 132 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,15 @@ import type { PullRequestBranches } from "./actions-util";
66
import * as apiClient from "./api-client";
77
import {
88
getDiffInformedAnalysisBranches,
9+
prepareDiffInformedAnalysis,
910
exportedForTesting,
1011
} from "./diff-informed-analysis-utils";
11-
import { Feature, initFeatures } from "./feature-flags";
12+
import { Feature, FeatureEnablement, initFeatures } from "./feature-flags";
1213
import { getRunnerLogger } from "./logging";
1314
import { parseRepositoryNwo } from "./repository";
1415
import {
1516
setupTests,
17+
createFeatures,
1618
mockCodeQLVersion,
1719
mockFeatureFlagApiEndpoint,
1820
setupActionsVars,
@@ -187,6 +189,135 @@ test.serial(
187189
false,
188190
);
189191

192+
test.serial(
193+
"prepareDiffInformedAnalysis: returns shouldRun=false when not a pull request",
194+
async (t) => {
195+
await withTmpDir(async (tmpDir) => {
196+
setupActionsVars(tmpDir, tmpDir);
197+
const logger = getRunnerLogger(true);
198+
const codeql = mockCodeQLVersion("2.21.0");
199+
const features = createFeatures([Feature.DiffInformedQueries]);
200+
201+
sinon.stub(actionsUtil, "getPullRequestBranches").returns(undefined);
202+
sinon
203+
.stub(apiClient, "getGitHubVersion")
204+
.resolves({ type: GitHubVariant.DOTCOM });
205+
206+
const result = await prepareDiffInformedAnalysis(
207+
codeql,
208+
features,
209+
logger,
210+
);
211+
212+
t.deepEqual(result, { shouldRun: false, hasDiffRanges: false });
213+
});
214+
},
215+
);
216+
217+
test.serial(
218+
"prepareDiffInformedAnalysis: returns shouldRun=false when applicability check throws",
219+
async (t) => {
220+
await withTmpDir(async (tmpDir) => {
221+
setupActionsVars(tmpDir, tmpDir);
222+
const logger = getRunnerLogger(true);
223+
const codeql = mockCodeQLVersion("2.21.0");
224+
// A features implementation whose getValue rejects, simulating an
225+
// unexpected failure when determining whether diff-informed analysis
226+
// should run.
227+
const features: FeatureEnablement = {
228+
getDefaultCliVersion: async () => {
229+
throw new Error("not implemented");
230+
},
231+
getValue: async () => {
232+
throw new Error("feature flag lookup failed");
233+
},
234+
};
235+
236+
const result = await prepareDiffInformedAnalysis(
237+
codeql,
238+
features,
239+
logger,
240+
);
241+
242+
t.deepEqual(result, { shouldRun: false, hasDiffRanges: false });
243+
});
244+
},
245+
);
246+
247+
test.serial(
248+
"prepareDiffInformedAnalysis: returns hasDiffRanges=true when the diff is fetched successfully",
249+
async (t) => {
250+
await withTmpDir(async (tmpDir) => {
251+
setupActionsVars(tmpDir, tmpDir);
252+
const logger = getRunnerLogger(true);
253+
const codeql = mockCodeQLVersion("2.21.0");
254+
const features = createFeatures([Feature.DiffInformedQueries]);
255+
256+
sinon
257+
.stub(actionsUtil, "getPullRequestBranches")
258+
.returns({ base: "main", head: "feature" });
259+
sinon
260+
.stub(apiClient, "getGitHubVersion")
261+
.resolves({ type: GitHubVariant.DOTCOM });
262+
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
263+
sinon.stub(apiClient, "getApiClient").returns({
264+
rest: {
265+
repos: {
266+
compareCommitsWithBasehead: sinon
267+
.stub()
268+
.resolves({ data: { files: [] } }),
269+
},
270+
},
271+
} as any);
272+
273+
const result = await prepareDiffInformedAnalysis(
274+
codeql,
275+
features,
276+
logger,
277+
);
278+
279+
t.deepEqual(result, { shouldRun: true, hasDiffRanges: true });
280+
});
281+
},
282+
);
283+
284+
test.serial(
285+
"prepareDiffInformedAnalysis: returns hasDiffRanges=false when the diff API call fails",
286+
async (t) => {
287+
await withTmpDir(async (tmpDir) => {
288+
setupActionsVars(tmpDir, tmpDir);
289+
const logger = getRunnerLogger(true);
290+
const codeql = mockCodeQLVersion("2.21.0");
291+
const features = createFeatures([Feature.DiffInformedQueries]);
292+
293+
sinon
294+
.stub(actionsUtil, "getPullRequestBranches")
295+
.returns({ base: "main", head: "feature" });
296+
sinon
297+
.stub(apiClient, "getGitHubVersion")
298+
.resolves({ type: GitHubVariant.DOTCOM });
299+
const notFoundError: any = new Error("Not Found");
300+
notFoundError.status = 404;
301+
// eslint-disable-next-line @typescript-eslint/no-unsafe-argument
302+
sinon.stub(apiClient, "getApiClient").returns({
303+
rest: {
304+
repos: {
305+
compareCommitsWithBasehead: sinon.stub().rejects(notFoundError),
306+
},
307+
},
308+
} as any);
309+
310+
const result = await prepareDiffInformedAnalysis(
311+
codeql,
312+
features,
313+
logger,
314+
);
315+
316+
t.deepEqual(result, { shouldRun: true, hasDiffRanges: false });
317+
});
318+
},
319+
);
320+
190321
function runGetDiffRanges(changes: number, patch: string[] | undefined): any {
191322
return exportedForTesting.getDiffRanges(
192323
{

0 commit comments

Comments
 (0)