forked from benchmark-action/github-action-benchmark
-
Notifications
You must be signed in to change notification settings - Fork 0
Ankitml/git graph awareness #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
239cd5d
feat: Implement git-graph aware benchmark sorting
ankitml 58cb4b5
test: Add unit tests for git-graph functionality
ankitml 39095e0
gitgraph
ankitml 9f0ddca
CI check
ankitml daa027d
Update src/gitGraph.ts
ankitml 1d2ae9f
stu's comment
ankitml ca7156c
lint
ankitml 941cedb
more fixes
ankitml 89af8c3
comments
ankitml 15b0628
better organization
ankitml File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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')); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's rm this unnecessary diff