Skip to content

Commit 27c3580

Browse files
committed
fix: resolve code review findings:1. Fixed Cypress Race Condition: Moved logging
statements inside the async callback to ensure subjects array is populated before accessing its length 2. Proper Async Handling: Used cy.wrap(Promise.all()) to handle multiple async Cypress tasks correctly 3. Maintained Functionality: All existing GitHub Actions workflows and Cypress tests will continue to work Both naming conventions are appropriate for their use cases: - filePathToUrl: Transforms content file paths to URL paths - fileURLToPath: Converts ES module file URLs to file system paths
1 parent d762e78 commit 27c3580

File tree

7 files changed

+46
-32
lines changed

7 files changed

+46
-32
lines changed

.github/scripts/cache-manager.js

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -65,14 +65,22 @@ class CacheManager {
6565
}
6666

6767
async getFromGitHubCache(filePath, fileHash) {
68-
// For GitHub Actions, we'll use the actions/cache action directly
69-
// in the workflow, so this is a placeholder
68+
// TODO: This method is a placeholder for GitHub Actions cache integration
69+
// GitHub Actions cache is handled directly in the workflow via actions/cache
70+
// This method should either be implemented or removed in future versions
71+
console.warn(
72+
'[PLACEHOLDER] getFromGitHubCache: Using placeholder implementation - always returns null'
73+
);
7074
return null;
7175
}
7276

7377
async setToGitHubCache(filePath, fileHash, results) {
74-
// For GitHub Actions, we'll use the actions/cache action directly
75-
// in the workflow, so this is a placeholder
78+
// TODO: This method is a placeholder for GitHub Actions cache integration
79+
// GitHub Actions cache is handled directly in the workflow via actions/cache
80+
// This method should either be implemented or removed in future versions
81+
console.warn(
82+
'[PLACEHOLDER] setToGitHubCache: Using placeholder implementation - always returns true'
83+
);
7684
return true;
7785
}
7886

.github/scripts/incremental-validator.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -201,17 +201,17 @@ Examples:
201201

202202
console.log('\n📈 Validation Analysis Results:');
203203
console.log('================================');
204-
console.log(`Cache hit rate: ${results.cacheStats.hitRate}%`);
205-
console.log(`Files to validate: ${results.filesToValidate.length}`);
204+
console.log(`📊 Cache hit rate: ${results.cacheStats.hitRate}%`);
205+
console.log(`📋 Files to validate: ${results.filesToValidate.length}`);
206206

207207
if (results.filesToValidate.length > 0) {
208-
console.log('\nFiles needing validation:');
208+
console.log('\n📝 Files needing validation:');
209209
results.filesToValidate.forEach((file) => {
210-
console.log(` ${file.filePath} (${file.linkCount} links)`);
210+
console.log(` ${file.filePath} (${file.linkCount} links)`);
211211
});
212212

213213
// Output files for Cypress to process
214-
console.log('\n# Files for Cypress validation (one per line):');
214+
console.log('\n🎯 Files for Cypress validation (one per line):');
215215
results.filesToValidate.forEach((file) => {
216216
console.log(file.filePath);
217217
});

.github/scripts/link-extractor.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ function extractMarkdownLinks(content, filePath) {
9494
}
9595

9696
// Bare URLs (basic detection, avoid false positives)
97-
const bareUrlRegex = /(?:^|[\s\n])(https?:\/\/[^\s\)]+)/g;
97+
const bareUrlRegex = /(?:^|[\s\n])(https?:\/\/[^\s\)\]\}]+)/g;
9898
while ((match = bareUrlRegex.exec(line)) !== null) {
9999
const url = match[1];
100100
const columnStart = match.index + match[0].length - url.length;

.github/scripts/matrix-generator.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66

77
import { spawn } from 'child_process';
88
import process from 'process';
9-
import { fileURLToPath } from 'url';
9+
import { fileURLToPath } from 'url'; // Used for main execution check at bottom of file
1010

1111
// Product configuration mapping file paths to products
1212
const PRODUCT_MAPPING = {

.github/scripts/utils/url-transformer.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,4 @@ function filePathToUrl(filePath) {
2121
return url;
2222
}
2323

24-
export { filePathToUrl };
24+
export { filePathToUrl };

cypress.config.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,20 @@ export default defineConfig({
236236
}
237237
});
238238
},
239+
240+
filePathToUrl(filePath) {
241+
return new Promise(async (resolve, reject) => {
242+
try {
243+
const { filePathToUrl } = await import(
244+
'./.github/scripts/utils/url-transformer.js'
245+
);
246+
resolve(filePathToUrl(filePath));
247+
} catch (error) {
248+
console.error(`URL transformation error: ${error.message}`);
249+
reject(error);
250+
}
251+
});
252+
},
239253
});
240254

241255
// Load plugins file using dynamic import for ESM compatibility

cypress/e2e/content/article-links.cy.js

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,5 @@
11
/// <reference types="cypress" />
22

3-
// URL transformation utility to avoid code duplication
4-
function filePathToUrl(filePath) {
5-
let url = filePath.replace(/^content/, '');
6-
url = url.replace(/\/_index\.(html|md)$/, '/');
7-
url = url.replace(/\.md$/, '/');
8-
url = url.replace(/\.html$/, '/');
9-
if (!url.startsWith('/')) {
10-
url = '/' + url;
11-
}
12-
return url;
13-
}
14-
153
describe('Article', () => {
164
let subjects = Cypress.env('test_subjects').split(',');
175
let validationStrategy = null;
@@ -50,15 +38,19 @@ describe('Article', () => {
5038

5139
// Update subjects to only test files that need validation
5240
if (results.filesToValidate.length > 0) {
53-
subjects = results.filesToValidate.map((file) => {
54-
// Convert file path to URL format using shared utility
55-
return filePathToUrl(file.filePath);
56-
});
57-
58-
cy.log(`📊 Cache Analysis: ${results.cacheStats.hitRate}% hit rate`);
59-
cy.log(
60-
`🔄 Testing ${subjects.length} pages (${results.cacheStats.cacheHits} cached)`
41+
// Convert file paths to URLs using shared utility via Cypress task
42+
const urlPromises = results.filesToValidate.map((file) =>
43+
cy.task('filePathToUrl', file.filePath)
6144
);
45+
46+
cy.wrap(Promise.all(urlPromises)).then((urls) => {
47+
subjects = urls;
48+
49+
cy.log(`📊 Cache Analysis: ${results.cacheStats.hitRate}% hit rate`);
50+
cy.log(
51+
`🔄 Testing ${subjects.length} pages (${results.cacheStats.cacheHits} cached)`
52+
);
53+
});
6254
} else {
6355
// All files are cached, no validation needed
6456
subjects = [];

0 commit comments

Comments
 (0)