Skip to content

Commit 8677c70

Browse files
Copilothsluoyz
andcommitted
fix: address code review feedback
- Fix usage message in extract-results.js and compare-results.js - Add file existence checks in benchmark.ts to validate example files - Improve error handling in workflow using continue-on-error - Make benchmark failures more visible while still allowing comparison Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
1 parent 38f2ab6 commit 8677c70

4 files changed

Lines changed: 44 additions & 10 deletions

File tree

.github/workflows/benchmark.yml

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,17 @@ jobs:
3333

3434
- name: Run PR benchmarks
3535
id: pr-benchmark
36+
continue-on-error: true
3637
run: |
3738
echo "Running benchmarks on PR branch..."
3839
yarn benchmark > pr-benchmark.txt 2>&1
3940
EXIT_CODE=$?
4041
cat pr-benchmark.txt
41-
if [ $EXIT_CODE -ne 0 ]; then
42-
echo "⚠️ Benchmark run failed with exit code $EXIT_CODE"
43-
fi
44-
exit 0
42+
exit $EXIT_CODE
43+
44+
- name: Check PR benchmark status
45+
if: steps.pr-benchmark.outcome == 'failure'
46+
run: echo "⚠️ PR benchmarks failed - will attempt to extract partial results"
4547

4648
- name: Extract PR results
4749
id: pr-results
@@ -60,15 +62,17 @@ jobs:
6062

6163
- name: Run base benchmarks
6264
id: base-benchmark
65+
continue-on-error: true
6366
run: |
6467
echo "Running benchmarks on base branch..."
6568
yarn benchmark > base-benchmark.txt 2>&1
6669
EXIT_CODE=$?
6770
cat base-benchmark.txt
68-
if [ $EXIT_CODE -ne 0 ]; then
69-
echo "⚠️ Benchmark run failed with exit code $EXIT_CODE"
70-
fi
71-
exit 0
71+
exit $EXIT_CODE
72+
73+
- name: Check base benchmark status
74+
if: steps.base-benchmark.outcome == 'failure'
75+
run: echo "⚠️ Base benchmarks failed - will attempt to extract partial results"
7276

7377
- name: Extract base results
7478
id: base-results

benchmark/benchmark.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
// limitations under the License.
1414

1515
import Benchmark from 'benchmark';
16+
import * as fs from 'fs';
17+
import * as path from 'path';
1618

1719
// Use CommonJS require to import from built library
1820
// eslint-disable-next-line @typescript-eslint/no-var-requires
@@ -28,6 +30,31 @@ interface BenchmarkResult {
2830

2931
const results: BenchmarkResult[] = [];
3032

33+
// Verify required example files exist
34+
function checkExampleFiles(): void {
35+
const requiredFiles = [
36+
'examples/rbac_model.conf',
37+
'examples/rbac_policy.csv',
38+
'examples/abac_model.conf',
39+
'examples/basic_model.conf',
40+
'examples/basic_policy.csv',
41+
];
42+
43+
const missingFiles: string[] = [];
44+
for (const file of requiredFiles) {
45+
if (!fs.existsSync(path.join(process.cwd(), file))) {
46+
missingFiles.push(file);
47+
}
48+
}
49+
50+
if (missingFiles.length > 0) {
51+
console.error('Error: Required example files not found:');
52+
missingFiles.forEach((file) => console.error(` - ${file}`));
53+
console.error('\nPlease ensure you are running this from the repository root directory.');
54+
process.exit(1);
55+
}
56+
}
57+
3158
async function setupEnforcers(): Promise<{
3259
rbacEnforcer: any;
3360
abacEnforcer: any;
@@ -63,6 +90,9 @@ function createSuite(name: string): Benchmark.Suite {
6390
}
6491

6592
async function runBenchmarks(): Promise<void> {
93+
// Check that all required files exist
94+
checkExampleFiles();
95+
6696
console.log('Setting up enforcers...');
6797
const { rbacEnforcer, abacEnforcer, basicEnforcer } = await setupEnforcers();
6898

benchmark/compare-results.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
const fs = require('fs');
66

77
if (process.argv.length < 4) {
8-
console.error('Usage: compare-results.js <base-results.json> <pr-results.json>');
8+
console.error('Usage: node compare-results.js <base-results.json> <pr-results.json>');
99
process.exit(1);
1010
}
1111

benchmark/extract-results.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
const fs = require('fs');
66

77
if (process.argv.length < 4) {
8-
console.error('Usage: extract-benchmark-results.js <input-file> <output-file>');
8+
console.error('Usage: node extract-results.js <input-file> <output-file>');
99
process.exit(1);
1010
}
1111

0 commit comments

Comments
 (0)