diff --git a/src/addBenchmarkEntry.ts b/src/addBenchmarkEntry.ts index da2996d72..6f8318fbb 100644 --- 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,6 +12,7 @@ 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) { @@ -18,17 +20,24 @@ export function addBenchmarkEntry( 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 index 0ce3bd5c0..e3942d112 100644 --- 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 index a6246d51b..ddc423fa1 100644 --- a/src/default_index_html.ts +++ b/src/default_index_html.ts @@ -161,11 +161,14 @@ export const DEFAULT_INDEX_HTML = String.raw` 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 new file mode 100644 index 000000000..7a253decf --- /dev/null +++ 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 index 4e9ebad6c..d81876461 100644 --- 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 new file mode 100644 index 000000000..941db08be --- /dev/null +++ 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 new file mode 100644 index 000000000..4cb3d55a5 --- /dev/null +++ b/test/gitGraphAnalyzer.test.ts @@ -0,0 +1,276 @@ +// Mock modules before imports +const mockDebug = jest.fn(); +const mockWarning = jest.fn(); +const mockExecSync = jest.fn(); + +jest.mock('@actions/core', () => ({ + debug: mockDebug, + warning: mockWarning, +})); + +jest.mock('child_process', () => ({ + execSync: mockExecSync, +})); + +import { GitGraphAnalyzer } from '../src/gitGraph'; +import { Benchmark } from '../src/extract'; + +describe('GitGraphAnalyzer', () => { + let analyzer: GitGraphAnalyzer; + const originalEnv = process.env; + + beforeEach(() => { + jest.clearAllMocks(); + // Reset environment + process.env = { ...originalEnv }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + describe('constructor', () => { + it('should detect git CLI availability', () => { + mockExecSync.mockReturnValue('git version 2.40.0'); + analyzer = new GitGraphAnalyzer(); + expect(analyzer.isGitAvailable()).toBe(true); + }); + + it('should detect git CLI unavailability', () => { + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') { + throw new Error('Command not found'); + } + return ''; + }); + + analyzer = new GitGraphAnalyzer(); + expect(analyzer.isGitAvailable()).toBe(false); + }); + }); + + describe('getAncestry', () => { + beforeEach(() => { + mockExecSync.mockReturnValue('git version 2.40.0'); + process.env.GITHUB_WORKSPACE = '/github/workspace'; + analyzer = new GitGraphAnalyzer(); + }); + + it('should parse git log output correctly', () => { + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'abc123 Commit message 1\ndef456 Commit message 2\nghi789 Commit message 3'; + }); + analyzer = new GitGraphAnalyzer(); + + const ancestry = analyzer.getAncestry('main'); + + expect(mockExecSync).toHaveBeenCalledWith( + 'git log --oneline --topo-order main', + expect.objectContaining({ + encoding: 'utf8', + cwd: '/github/workspace', + }), + ); + expect(ancestry).toEqual(['abc123', 'def456', 'ghi789']); + }); + + it('should handle empty git log output', () => { + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return ''; + }); + analyzer = new GitGraphAnalyzer(); + + const ancestry = analyzer.getAncestry('main'); + expect(ancestry).toEqual([]); + }); + + it('should handle git command failure', () => { + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + throw new Error('Git command failed'); + }); + analyzer = new GitGraphAnalyzer(); + + const ancestry = analyzer.getAncestry('main'); + expect(ancestry).toEqual([]); + expect(mockWarning).toHaveBeenCalledWith(expect.stringContaining('Failed to get ancestry for ref main')); + }); + + it('should return empty array when git CLI not available', () => { + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') { + throw new Error('Command not found'); + } + return ''; + }); + analyzer = new GitGraphAnalyzer(); + + const ancestry = analyzer.getAncestry('main'); + expect(ancestry).toEqual([]); + expect(mockWarning).toHaveBeenCalledWith('Git CLI not available, cannot determine ancestry'); + }); + }); + + describe('findPreviousBenchmark', () => { + const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({ + commit: { + id, + timestamp, + message: `Commit ${id}`, + url: `https://github.com/test/repo/commit/${id}`, + author: { username: 'testuser' }, + committer: { username: 'testuser' }, + }, + date: Date.now(), + tool: 'cargo', + benches: [], + }); + + beforeEach(() => { + process.env.GITHUB_WORKSPACE = '/github/workspace'; + }); + + it('should find previous benchmark using git ancestry', () => { + const suites = [ + createMockBenchmark('abc123', '2025-01-01T00:00:00Z'), + createMockBenchmark('def456', '2025-01-02T00:00:00Z'), + createMockBenchmark('ghi789', '2025-01-03T00:00:00Z'), + ]; + + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1'; + }); + analyzer = new GitGraphAnalyzer(); + + const result = analyzer.findPreviousBenchmark(suites, 'ghi789'); + + expect(result?.commit.id).toBe('def456'); + expect(result?.commit.timestamp).toBe('2025-01-02T00:00:00Z'); + expect(mockDebug).toHaveBeenCalledWith('Found previous benchmark: def456 based on git ancestry'); + }); + + it('should return null when no previous benchmark found', () => { + const suites = [createMockBenchmark('abc123', '2025-01-01T00:00:00Z')]; + + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'abc123 Commit 1'; + }); + analyzer = new GitGraphAnalyzer(); + + const result = analyzer.findPreviousBenchmark(suites, 'abc123'); + + expect(result).toBeNull(); + expect(mockDebug).toHaveBeenCalledWith('No previous benchmark found in git ancestry'); + }); + + it('should fallback to execution time when git command fails', () => { + const suites = [ + createMockBenchmark('abc123', '2025-01-01T00:00:00Z'), + createMockBenchmark('def456', '2025-01-02T00:00:00Z'), + ]; + + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + throw new Error('Git failed'); + }); + analyzer = new GitGraphAnalyzer(); + + const result = analyzer.findPreviousBenchmark(suites, 'def456'); + + // Should fallback to execution time logic (previous in array) + expect(result?.commit.id).toBe('abc123'); + expect(result?.commit.timestamp).toBe('2025-01-01T00:00:00Z'); + }); + + it('should fallback to execution time when current commit not in ancestry', () => { + const suites = [ + createMockBenchmark('abc123', '2025-01-01T00:00:00Z'), + createMockBenchmark('def456', '2025-01-02T00:00:00Z'), + ]; + + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'xyz999 Other commit'; + }); + analyzer = new GitGraphAnalyzer(); + + const result = analyzer.findPreviousBenchmark(suites, 'def456'); + + // Should fallback to execution time logic + expect(result?.commit.id).toBe('abc123'); + expect(result?.commit.timestamp).toBe('2025-01-01T00:00:00Z'); + }); + }); + + describe('findInsertionIndex', () => { + const createMockBenchmark = (id: string, timestamp: string): Benchmark => ({ + commit: { + id, + timestamp, + message: `Commit ${id}`, + url: `https://github.com/test/repo/commit/${id}`, + author: { username: 'testuser' }, + committer: { username: 'testuser' }, + }, + date: Date.now(), + tool: 'cargo', + benches: [], + }); + + beforeEach(() => { + process.env.GITHUB_WORKSPACE = '/github/workspace'; + }); + + it('should find correct insertion index after ancestor', () => { + const suites = [ + createMockBenchmark('abc123', '2025-01-01T00:00:00Z'), + createMockBenchmark('def456', '2025-01-02T00:00:00Z'), + ]; + + // New commit ghi789 has def456 as ancestor + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'ghi789 Commit 3\ndef456 Commit 2\nabc123 Commit 1'; + }); + analyzer = new GitGraphAnalyzer(); + + const index = analyzer.findInsertionIndex(suites, 'ghi789'); + + // Should insert after def456 (index 1), so at index 2 + expect(index).toBe(2); + expect(mockDebug).toHaveBeenCalledWith('Found ancestor def456 at index 1, inserting after it'); + }); + + it('should append to end when no ancestor found', () => { + const suites = [ + createMockBenchmark('abc123', '2025-01-01T00:00:00Z'), + createMockBenchmark('def456', '2025-01-02T00:00:00Z'), + ]; + + // New commit has no relation to existing commits + mockExecSync.mockImplementation((cmd: string) => { + if (cmd === 'git --version') return 'git version 2.40.0'; + return 'xyz999 Unrelated commit'; + }); + analyzer = new GitGraphAnalyzer(); + + const index = analyzer.findInsertionIndex(suites, 'xyz999'); + + expect(index).toBe(2); + expect(mockDebug).toHaveBeenCalledWith('No ancestor found in existing suites, appending to end'); + }); + + it('should append to end for empty suites', () => { + mockExecSync.mockReturnValue('git version 2.40.0'); + analyzer = new GitGraphAnalyzer(); + + const index = analyzer.findInsertionIndex([], 'abc123'); + + expect(index).toBe(0); + }); + }); +}); diff --git a/test/write.spec.ts b/test/write.spec.ts index 63611146a..7dd778014 100644 --- a/test/write.spec.ts +++ b/test/write.spec.ts @@ -116,6 +116,20 @@ jest.mock('../src/git', () => ({ }, })); +jest.mock('../src/gitGraph', () => ({ + GitGraphAnalyzer: jest.fn().mockImplementation(() => ({ + getCurrentBranch: () => 'main', + findPreviousBenchmark: (suites: any[]) => { + if (suites.length > 0) { + return suites[suites.length - 1]; + } + return null; + }, + findInsertionIndex: (suites: any[]) => suites.length, + sortByGitOrder: (suites: any[]) => suites, + })), +})); + describe.each(['https://github.com', 'https://github.enterprise.corp'])('writeBenchmark() - %s', function (serverUrl) { const savedCwd = process.cwd();