Merge pull request #3 from paradedb/ankitml/git-graph-awareness

ankitml · web-flow · commit e936a38c6609 · 2026-01-06T02:54:01.000+05:30
Ankitml/git graph awareness
diff --git a/src/addBenchmarkEntry.ts b/src/addBenchmarkEntry.ts
@@ -2,6 +2,7 @@ import { Benchmark } from './extract';
 import * as core from '@actions/core';
 import { BenchmarkSuites } from './write';
 import { normalizeBenchmark } from './normalizeBenchmark';
+import { GitGraphAnalyzer } from './gitGraph';
 
 export function addBenchmarkEntry(
     benchName: string,
@@ -11,24 +12,32 @@ export function addBenchmarkEntry(
 ): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } {
     let prevBench: Benchmark | null = null;
     let normalizedCurrentBench: Benchmark = benchEntry;
+    const gitAnalyzer = new GitGraphAnalyzer();
 
     // Add benchmark result
     if (entries[benchName] === undefined) {
         entries[benchName] = [benchEntry];
         core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);
     } else {
         const suites = entries[benchName];
-        // Get the last suite which has different commit ID for alert comment
-        for (const e of [...suites].reverse()) {
-            if (e.commit.id !== benchEntry.commit.id) {
-                prevBench = e;
-                break;
-            }
+
+        // Find previous benchmark using git ancestry
+        core.debug(`Finding previous benchmark for commit: ${benchEntry.commit.id}`);
+
+        prevBench = gitAnalyzer.findPreviousBenchmark(suites, benchEntry.commit.id);
+
+        if (prevBench) {
+            core.debug(`Found previous benchmark: ${prevBench.commit.id}`);
+        } else {
+            core.debug('No previous benchmark found');
         }
 
         normalizedCurrentBench = normalizeBenchmark(prevBench, benchEntry);
 
-        suites.push(normalizedCurrentBench);
+        // Insert at the correct position based on git ancestry
+        const insertionIndex = gitAnalyzer.findInsertionIndex(suites, benchEntry.commit.id);
+        core.debug(`Inserting benchmark at index ${insertionIndex} (of ${suites.length} existing entries)`);
+        suites.splice(insertionIndex, 0, normalizedCurrentBench);
 
         if (maxItems !== null && suites.length > maxItems) {
             suites.splice(0, suites.length - maxItems);
diff --git a/src/config.ts b/src/config.ts
@@ -4,6 +4,7 @@ import * as os from 'os';
 import * as path from 'path';
 
 export type ToolType = typeof VALID_TOOLS[number];
+
 export interface Config {
     name: string;
     tool: ToolType;
diff --git a/src/default_index_html.ts b/src/default_index_html.ts
@@ -161,11 +161,14 @@ export const DEFAULT_INDEX_HTML = String.raw`<!DOCTYPE html>
             a.click();
           };
 
-          // Prepare data points for charts
-          return Object.keys(data.entries).map(name => ({
-            name,
-            dataSet: collectBenchesPerTestCase(data.entries[name]),
-          }));
+          // Prepare data points for charts (uses server-side ordering)
+          return Object.keys(data.entries).map(name => {
+            const entries = data.entries[name];
+            return {
+              name,
+              dataSet: collectBenchesPerTestCase(entries),
+            };
+          });
         }
 
         function renderAllChars(dataSets) {
diff --git a/src/gitGraph.ts b/src/gitGraph.ts
@@ -0,0 +1,129 @@
+import { execSync } from 'child_process';
+import * as core from '@actions/core';
+import { Benchmark } from './extract';
+
+export class GitGraphAnalyzer {
+    private readonly gitCliAvailable: boolean;
+
+    constructor() {
+        try {
+            execSync('git --version', { stdio: 'ignore' });
+            this.gitCliAvailable = true;
+        } catch (e) {
+            this.gitCliAvailable = false;
+        }
+    }
+
+    /**
+     * Check if git CLI is available
+     */
+    isGitAvailable(): boolean {
+        return this.gitCliAvailable;
+    }
+
+    /**
+     * Get git ancestry using topological order
+     */
+    getAncestry(ref: string): string[] {
+        if (!this.gitCliAvailable) {
+            core.warning('Git CLI not available, cannot determine ancestry');
+            return [];
+        }
+
+        try {
+            const output = execSync(`git log --oneline --topo-order ${ref}`, {
+                encoding: 'utf8',
+                cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(),
+            });
+
+            return output
+                .split('\n')
+                .filter((line) => line.trim())
+                .map((line) => line.split(' ')[0]); // Extract SHA from "sha message"
+        } catch (error) {
+            core.warning(`Failed to get ancestry for ref ${ref}: ${error}`);
+            return [];
+        }
+    }
+
+    /**
+     * Find previous benchmark commit based on git ancestry.
+     * Falls back to execution time ordering if git ancestry is not available.
+     */
+    findPreviousBenchmark(suites: Benchmark[], currentSha: string): Benchmark | null {
+        const ancestry = this.getAncestry(currentSha);
+
+        if (ancestry.length === 0) {
+            core.warning(`No ancestry found for commit ${currentSha}, falling back to execution time ordering`);
+            return this.findPreviousByExecutionTime(suites, currentSha);
+        }
+
+        // Find position of current commit in ancestry
+        const currentIndex = ancestry.indexOf(currentSha);
+        if (currentIndex === -1) {
+            core.warning(`Current commit ${currentSha} not found in ancestry, falling back to execution time ordering`);
+            return this.findPreviousByExecutionTime(suites, currentSha);
+        }
+
+        // Look for next commit in ancestry that exists in benchmarks
+        for (let i = currentIndex + 1; i < ancestry.length; i++) {
+            const previousSha = ancestry[i];
+            const previousBenchmark = suites.find((suite) => suite.commit.id === previousSha);
+
+            if (previousBenchmark) {
+                core.debug(`Found previous benchmark: ${previousSha} based on git ancestry`);
+                return previousBenchmark;
+            }
+        }
+
+        // Fallback: no previous commit found in ancestry
+        core.debug('No previous benchmark found in git ancestry');
+        return null;
+    }
+
+    /**
+     * Find the insertion index for a new benchmark entry based on git ancestry.
+     * Inserts after the most recent ancestor in the existing suites.
+     */
+    findInsertionIndex(suites: Benchmark[], newCommitSha: string): number {
+        if (!this.gitCliAvailable || suites.length === 0) {
+            return suites.length;
+        }
+
+        const ancestry = this.getAncestry(newCommitSha);
+        if (ancestry.length === 0) {
+            core.debug('No ancestry found, appending to end');
+            return suites.length;
+        }
+
+        // Create a set of ancestor SHAs for quick lookup (excluding the commit itself)
+        const ancestorSet = new Set(ancestry.slice(1)); // Skip first element (the commit itself)
+
+        // Find the most recent ancestor in the existing suites
+        // Iterate through suites from end to beginning to find the most recent one
+        for (let i = suites.length - 1; i >= 0; i--) {
+            const suite = suites[i];
+            if (ancestorSet.has(suite.commit.id)) {
+                core.debug(`Found ancestor ${suite.commit.id} at index ${i}, inserting after it`);
+                return i + 1; // Insert after this ancestor
+            }
+        }
+
+        // No ancestor found in existing suites - this commit is likely from a different branch
+        // or is very old. Append to end as fallback.
+        core.debug('No ancestor found in existing suites, appending to end');
+        return suites.length;
+    }
+
+    /**
+     * Fallback method: find previous by execution time (original logic)
+     */
+    private findPreviousByExecutionTime(suites: Benchmark[], currentSha: string): Benchmark | null {
+        for (const suite of [...suites].reverse()) {
+            if (suite.commit.id !== currentSha) {
+                return suite;
+            }
+        }
+        return null;
+    }
+}
diff --git a/src/write.ts b/src/write.ts
@@ -490,7 +490,7 @@ async function writeBenchmarkToExternalJson(
     jsonFilePath: string,
     config: Config,
 ): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> {
-    const { name, maxItemsInChart, saveDataFile } = config;
+    const { name, saveDataFile, maxItemsInChart } = config;
     const data = await loadDataJson(jsonFilePath);
     const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);
 
diff --git a/test/addBenchmarkEntryGitGraph.test.ts b/test/addBenchmarkEntryGitGraph.test.ts
@@ -0,0 +1,170 @@
+// Mock modules before imports
+const mockDebug = jest.fn();
+const mockFindPreviousBenchmark = jest.fn();
+const mockFindInsertionIndex = jest.fn();
+
+jest.mock('@actions/core', () => ({
+    debug: mockDebug,
+}));
+
+jest.mock('../src/gitGraph', () => ({
+    GitGraphAnalyzer: jest.fn().mockImplementation(() => ({
+        findPreviousBenchmark: mockFindPreviousBenchmark,
+        findInsertionIndex: mockFindInsertionIndex,
+    })),
+}));
+
+import { addBenchmarkEntry } from '../src/addBenchmarkEntry';
+import { Benchmark } from '../src/extract';
+
+describe('addBenchmarkEntry with Git Graph', () => {
+    const createMockBenchmark = (id: string, timestamp?: string): Benchmark => ({
+        commit: {
+            id,
+            timestamp: timestamp ?? '2025-01-01T00:00:00Z',
+            message: `Commit ${id}`,
+            url: `https://github.com/test/repo/commit/${id}`,
+            author: { username: 'testuser' },
+            committer: { username: 'testuser' },
+        },
+        date: Date.now(),
+        tool: 'cargo',
+        benches: [
+            {
+                name: 'test_bench',
+                value: 100,
+                unit: 'ms',
+            },
+        ],
+    });
+
+    beforeEach(() => {
+        jest.clearAllMocks();
+    });
+
+    it('should use git graph analyzer to find previous benchmark', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('abc123');
+        const existingEntry = createMockBenchmark('def456');
+        const entries = {
+            [benchName]: [existingEntry],
+        };
+
+        mockFindPreviousBenchmark.mockReturnValue(existingEntry);
+        mockFindInsertionIndex.mockReturnValue(1);
+
+        const result = addBenchmarkEntry(benchName, benchEntry, entries, null);
+
+        expect(mockFindPreviousBenchmark).toHaveBeenCalledWith(expect.arrayContaining([existingEntry]), 'abc123');
+        expect(mockDebug).toHaveBeenCalledWith('Finding previous benchmark for commit: abc123');
+        expect(mockDebug).toHaveBeenCalledWith('Found previous benchmark: def456');
+        expect(result.prevBench).toBe(existingEntry);
+    });
+
+    it('should handle no previous benchmark found', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('abc123');
+        const existingEntry = createMockBenchmark('def456');
+        const entries = {
+            [benchName]: [existingEntry],
+        };
+
+        mockFindPreviousBenchmark.mockReturnValue(null);
+        mockFindInsertionIndex.mockReturnValue(1);
+
+        const result = addBenchmarkEntry(benchName, benchEntry, entries, null);
+
+        expect(mockDebug).toHaveBeenCalledWith('Finding previous benchmark for commit: abc123');
+        expect(mockDebug).toHaveBeenCalledWith('No previous benchmark found');
+        expect(result.prevBench).toBeNull();
+    });
+
+    it('should create new benchmark suite when none exists', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('abc123');
+        const entries: any = {};
+
+        mockFindPreviousBenchmark.mockReturnValue(null);
+
+        const result = addBenchmarkEntry(benchName, benchEntry, entries, null);
+
+        expect(entries[benchName]).toEqual([benchEntry]);
+        expect(result.prevBench).toBeNull();
+        expect(result.normalizedCurrentBench).toBe(benchEntry);
+        expect(mockDebug).toHaveBeenCalledWith(
+            "No suite was found for benchmark 'test-suite' in existing data. Created",
+        );
+    });
+
+    it('should add to existing benchmark suite', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('abc123');
+        const existingEntry = createMockBenchmark('def456');
+        const entries = {
+            [benchName]: [existingEntry],
+        };
+
+        mockFindPreviousBenchmark.mockReturnValue(existingEntry);
+        mockFindInsertionIndex.mockReturnValue(1);
+
+        const result = addBenchmarkEntry(benchName, benchEntry, entries, null);
+
+        expect(entries[benchName]).toHaveLength(2);
+        expect(entries[benchName][1]).toEqual(
+            expect.objectContaining({
+                commit: benchEntry.commit,
+                tool: benchEntry.tool,
+            }),
+        );
+        expect(result.prevBench).toBe(existingEntry);
+    });
+
+    it('should respect maxItems limit', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('new123');
+        const oldEntries = [createMockBenchmark('old1'), createMockBenchmark('old2'), createMockBenchmark('old3')];
+        const entries = {
+            [benchName]: oldEntries,
+        };
+
+        mockFindPreviousBenchmark.mockReturnValue(oldEntries[oldEntries.length - 1]);
+        mockFindInsertionIndex.mockReturnValue(3);
+
+        addBenchmarkEntry(benchName, benchEntry, entries, 3);
+
+        // Should have 3 items total (maxItems)
+        expect(entries[benchName]).toHaveLength(3);
+        expect(entries[benchName][2]).toEqual(
+            expect.objectContaining({
+                commit: benchEntry.commit,
+                tool: benchEntry.tool,
+            }),
+        );
+        // Should have removed oldest entries to maintain maxItems
+        // We started with 3 old entries + 1 new = 4, so maxItems=3 keeps the 3 most recent
+        expect(entries[benchName]).toHaveLength(3);
+        // The oldest entry (old1) should have been removed
+        expect(entries[benchName].map((e) => e.commit.id)).not.toContain('old1');
+        expect(mockDebug).toHaveBeenCalledWith(
+            "Number of data items for 'test-suite' was truncated to 3 due to max-items-in-charts",
+        );
+    });
+
+    it('should not truncate when under maxItems limit', () => {
+        const benchName = 'test-suite';
+        const benchEntry = createMockBenchmark('new123');
+        const oldEntries = [createMockBenchmark('old1')];
+        const entries = {
+            [benchName]: oldEntries,
+        };
+
+        mockFindPreviousBenchmark.mockReturnValue(oldEntries[0]);
+        mockFindInsertionIndex.mockReturnValue(1);
+
+        addBenchmarkEntry(benchName, benchEntry, entries, 5);
+
+        expect(entries[benchName]).toHaveLength(2);
+        // Should not call debug about truncation
+        expect(mockDebug).not.toHaveBeenCalledWith(expect.stringContaining('was truncated to'));
+    });
+});
diff --git a/test/gitGraphAnalyzer.test.ts b/test/gitGraphAnalyzer.test.ts
diff --git a/test/write.spec.ts b/test/write.spec.ts
