From 7e5479d295c1e70078a8df90c3a1f0e7c5a278ae Mon Sep 17 00:00:00 2001 From: Tobias Wilken Date: Fri, 2 Jan 2026 07:20:39 +0100 Subject: [PATCH] fix: improve code quality and test coverage - Add comprehensive tests for transfer permissions with fetch mocking - Add workflow timeouts (5-15 min) to prevent hanging jobs - Add file existence validation in parser before reading --- .github/workflows/drift-detection.yml | 1 + .github/workflows/sync-repositories.yml | 1 + .github/workflows/test.yml | 1 + scripts/check-transfer-permissions.test.js | 279 +++++++++++++++------ scripts/parse-repositories.js | 11 +- 5 files changed, 211 insertions(+), 82 deletions(-) diff --git a/.github/workflows/drift-detection.yml b/.github/workflows/drift-detection.yml index f73910a..4949f9f 100644 --- a/.github/workflows/drift-detection.yml +++ b/.github/workflows/drift-detection.yml @@ -6,6 +6,7 @@ on: jobs: detect-drift: runs-on: ubuntu-latest + timeout-minutes: 10 permissions: pull-requests: write contents: read diff --git a/.github/workflows/sync-repositories.yml b/.github/workflows/sync-repositories.yml index 0a3cd9c..84608a7 100644 --- a/.github/workflows/sync-repositories.yml +++ b/.github/workflows/sync-repositories.yml @@ -8,6 +8,7 @@ on: jobs: sync: runs-on: ubuntu-latest + timeout-minutes: 15 # Only run on the main worlddriven organization, not on forks if: github.repository_owner == 'worlddriven' permissions: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3d9749c..8ed59a8 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -13,6 +13,7 @@ on: jobs: test: runs-on: ubuntu-latest + timeout-minutes: 5 steps: - name: Checkout code diff --git a/scripts/check-transfer-permissions.test.js b/scripts/check-transfer-permissions.test.js index 34e5b56..59db1e5 100644 --- a/scripts/check-transfer-permissions.test.js +++ b/scripts/check-transfer-permissions.test.js @@ -1,96 +1,213 @@ #!/usr/bin/env node -import { describe, test } from 'node:test'; +import { describe, test, mock, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert'; -import { checkTransferPermission } from './check-transfer-permissions.js'; +import { + checkTransferPermission, + checkMultipleTransferPermissions, +} from './check-transfer-permissions.js'; describe('checkTransferPermission', () => { - test('should throw error if token is missing', async () => { - await assert.rejects( - async () => await checkTransferPermission(null, 'owner/repo'), - { message: 'GitHub token is required' } - ); + describe('input validation', () => { + test('should throw error if token is missing', async () => { + await assert.rejects( + async () => await checkTransferPermission(null, 'owner/repo'), + { message: 'GitHub token is required' } + ); + }); + + test('should throw error if originRepo is missing', async () => { + await assert.rejects( + async () => await checkTransferPermission('token', ''), + { message: 'Origin repository must be in format "owner/repo-name"' } + ); + }); + + test('should throw error if originRepo format is invalid', async () => { + await assert.rejects( + async () => await checkTransferPermission('token', 'invalid-format'), + { message: 'Origin repository must be in format "owner/repo-name"' } + ); + }); + + test('should throw error if originRepo has empty owner or repo', async () => { + await assert.rejects( + async () => await checkTransferPermission('token', '/repo'), + { message: 'Invalid origin repository format' } + ); + + await assert.rejects( + async () => await checkTransferPermission('token', 'owner/'), + { message: 'Invalid origin repository format' } + ); + }); }); - test('should throw error if originRepo is missing', async () => { - await assert.rejects( - async () => await checkTransferPermission('token', ''), - { message: 'Origin repository must be in format "owner/repo-name"' } - ); + describe('API response handling', () => { + let originalFetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; + }); + + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + test('should return hasPermission true when user has admin access', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + permissions: { admin: true, push: true, pull: true }, + }), + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, true); + assert.strictEqual(result.permissionLevel, 'admin'); + assert.ok(result.details.includes('admin access')); + }); + + test('should return hasPermission false when user has write access only', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + permissions: { admin: false, push: true, pull: true }, + }), + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'write'); + assert.ok(result.details.includes('admin required')); + }); + + test('should return hasPermission false when user has read access only', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + permissions: { admin: false, push: false, pull: true }, + }), + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'read'); + }); + + test('should handle 404 response for non-existent repository', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: false, + status: 404, + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'none'); + assert.ok(result.details.includes('not found')); + }); + + test('should handle API errors gracefully', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: false, + status: 403, + text: () => Promise.resolve('Rate limit exceeded'), + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'unknown'); + assert.ok(result.details.includes('403')); + }); + + test('should handle network errors gracefully', async () => { + globalThis.fetch = mock.fn(() => + Promise.reject(new Error('Network error')) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'error'); + assert.ok(result.details.includes('Network error')); + }); + + test('should handle missing permissions object in response', async () => { + globalThis.fetch = mock.fn(() => + Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve({}), + }) + ); + + const result = await checkTransferPermission('test-token', 'owner/repo'); + + assert.strictEqual(result.hasPermission, false); + assert.strictEqual(result.permissionLevel, 'none'); + }); + }); +}); + +describe('checkMultipleTransferPermissions', () => { + let originalFetch; + + beforeEach(() => { + originalFetch = globalThis.fetch; }); - test('should throw error if originRepo format is invalid', async () => { - await assert.rejects( - async () => await checkTransferPermission('token', 'invalid-format'), - { message: 'Origin repository must be in format "owner/repo-name"' } - ); + afterEach(() => { + globalThis.fetch = originalFetch; }); - test('should throw error if originRepo has empty owner or repo', async () => { - await assert.rejects( - async () => await checkTransferPermission('token', '/repo'), - { message: 'Invalid origin repository format' } - ); + test('should return a Map with results for all repositories', async () => { + let callCount = 0; + globalThis.fetch = mock.fn(() => { + callCount++; + return Promise.resolve({ + ok: true, + status: 200, + json: () => + Promise.resolve({ + permissions: { admin: callCount === 1, push: true, pull: true }, + }), + }); + }); - await assert.rejects( - async () => await checkTransferPermission('token', 'owner/'), - { message: 'Invalid origin repository format' } - ); + const repos = ['owner/repo1', 'owner/repo2']; + const results = await checkMultipleTransferPermissions('test-token', repos); + + assert.ok(results instanceof Map); + assert.strictEqual(results.size, 2); + assert.strictEqual(results.get('owner/repo1').hasPermission, true); + assert.strictEqual(results.get('owner/repo2').hasPermission, false); }); - // Note: The following tests would require mocking the fetch API - // or using a test GitHub token with known repositories. - // For now, we document the expected behavior: - - /** - * Test case for admin permission (success scenario): - * - Repository exists - * - worlddriven has admin access - * - Expected result: - * { - * hasPermission: true, - * permissionLevel: 'admin', - * details: '✅ worlddriven has admin access to owner/repo' - * } - */ - - /** - * Test case for write permission (insufficient): - * - Repository exists - * - worlddriven has write (but not admin) access - * - Expected result: - * { - * hasPermission: false, - * permissionLevel: 'write', - * details: '❌ worlddriven has "write" access to owner/repo (admin required)' - * } - */ - - /** - * Test case for non-existent repository: - * - Repository doesn't exist or worlddriven has no access - * - API returns 404 - * - Expected result: - * { - * hasPermission: false, - * permissionLevel: 'none', - * details: 'Repository owner/repo not found or worlddriven has no access' - * } - */ - - /** - * Test case for API errors: - * - Network errors, rate limits, etc. - * - Expected result: - * { - * hasPermission: false, - * permissionLevel: 'error' or 'unknown', - * details: 'Error checking permissions: ...' - * } - */ -}); + test('should return empty Map for empty input array', async () => { + const results = await checkMultipleTransferPermissions('test-token', []); -// To run integration tests with actual GitHub API: -// 1. Set WORLDDRIVEN_GITHUB_TOKEN environment variable -// 2. Create test repositories with known permission levels -// 3. Run: node --test scripts/check-transfer-permissions.test.js + assert.ok(results instanceof Map); + assert.strictEqual(results.size, 0); + }); +}); diff --git a/scripts/parse-repositories.js b/scripts/parse-repositories.js index d4d2221..850b119 100755 --- a/scripts/parse-repositories.js +++ b/scripts/parse-repositories.js @@ -5,7 +5,8 @@ * Returns an array of repository objects */ -import { readFile } from 'fs/promises'; +import { readFile, access } from 'fs/promises'; +import { constants } from 'fs'; import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; @@ -102,6 +103,14 @@ function parseRepositories(content) { async function parseRepositoriesFile() { const repoFilePath = join(__dirname, '..', 'REPOSITORIES.md'); + // Validate file exists before attempting to read + try { + await access(repoFilePath, constants.R_OK); + } catch { + console.error(`Error: REPOSITORIES.md not found at ${repoFilePath}`); + process.exit(1); + } + try { const content = await readFile(repoFilePath, 'utf-8'); return parseRepositories(content);