Development: Add e2e tests for git exercise participation using ssh and token
#10006
Conversation
github-actions
bot
added
tests
client
muradium
changed the title
Development: E2E test for exercise participation using git via SSHDevelopment: E2E tests for exercise participation using git via SSH
muradium
dismissed stale reviews from coolchock, BBesrour, and coderabbitai[bot]
via
13ad227
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/test/playwright/init/global-setup.ts (1)
1-24: Security: Review file operations outside Artemis directoryAs noted in previous reviews, the script operates on files outside the Artemis directory. This could pose security risks and make the testing environment less isolated.
Consider:
- Moving SSH keys within the Artemis test directory structure
- Adding validation to ensure file operations only occur within allowed paths
- Implementing a secure key management strategy that doesn't rely on filesystem operations
Would you like assistance in designing a more secure key management approach?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts(3 hunks)src/test/playwright/init/global-setup.ts(1 hunks)src/test/playwright/support/pageobjects/exercises/programming/GitClient.ts(1 hunks)
🔇 Additional comments (4)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (4)
8-21: LGTM: Import statements are well-organized
The new imports for SSH-related functionality are properly organized and necessary for the implementation.
84-91: LGTM: Test description is more specific
The test description now clearly indicates that it's testing HTTPS-based git submissions, improving test clarity.
300-311: Add error handling for SSH key setup
The function should handle potential errors when reading SSH key files and interacting with the UI.
This issue was previously identified in a past review. Please implement the suggested error handling.
92-120: 🛠️ Refactor suggestion
Add error handling for SSH key setup failures
While the test suite is well-structured, it should handle potential SSH key setup failures gracefully.
Apply this diff to add error handling:
test(`Makes a git submission using SSH with ${sshAlgorithm} key`, async ({ page, programmingExerciseOverview }) => {
+ try {
await programmingExerciseOverview.startParticipation(course.id!, exercise.id!, studentOne);
await makeGitExerciseSubmission(
page,
programmingExerciseOverview,
course,
exercise,
studentOne,
javaAllSuccessfulSubmission,
'Solution',
GitCloneMethod.ssh,
sshAlgorithm,
);
+ } catch (error) {
+ console.error(`Failed to setup SSH key (${sshAlgorithm}):`, error);
+ throw error;
+ }
});Also, verify that the SSH keys are properly configured:
src/test/playwright/support/pageobjects/exercises/programming/GitClient.ts
Show resolved
Hide resolved
src/test/playwright/support/pageobjects/exercises/programming/GitClient.ts
Show resolved
Hide resolved
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (2)
100-114: Enhance test descriptions with expected outcomesThe test descriptions could be more specific about what constitutes a successful test.
Apply this diff:
-test(`Makes a git submission using SSH with ${sshAlgorithm} key`, async ({ page, programmingExerciseOverview }) => { +test(`Successfully clones and pushes using SSH with ${sshAlgorithm} key, expecting successful build`, async ({ page, programmingExerciseOverview }) => {
281-282: Make CI URL replacement more robustThe URL replacement for CI environment could be more robust using URL parsing.
Apply this diff:
-if (process.env.CI === 'true' && (cloneMethod == GitCloneMethod.https || cloneMethod == GitCloneMethod.ssh)) { - repoUrl = repoUrl.replace(/(?<=@).+(?=:)/, 'artemis-app'); +if (process.env.CI === 'true') { + try { + const url = new URL(repoUrl.replace(':', '/').replace('git@', 'https://')); + url.hostname = 'artemis-app'; + repoUrl = url.toString() + .replace('https://', cloneMethod === GitCloneMethod.ssh ? 'git@' : 'https://') + .replace('/', ':'); + } catch (error) { + throw new Error(`Failed to parse repository URL: ${error.message}`); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts(3 hunks)
🔇 Additional comments (2)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (2)
304-315:
Add error handling and cleanup to SSH setup
The function needs better error handling and resource cleanup.
Previous review already noted the need for error handling when reading SSH key files. Additionally:
- Add SSH key format validation
- Ensure browser context cleanup
Apply this diff:
async function setupSSHCredentials(context: BrowserContext, sshAlgorithm: SshEncryptionAlgorithm) {
console.log(`Setting up SSH credentials with key ${SSH_KEY_NAMES[sshAlgorithm]}`);
const page = await context.newPage();
- const sshKeyPath = path.join(SSH_KEYS_PATH, `${SSH_KEY_NAMES[sshAlgorithm]}.pub`);
- const sshKey = await fs.readFile(sshKeyPath, 'utf8');
- await page.goto('user-settings/ssh');
- await page.getByTestId('addNewSshKeyButton').click();
- await page.getByTestId('sshKeyField').fill(sshKey!);
- const responsePromise = page.waitForResponse(`${BASE_API}/ssh-settings/public-key`);
- await page.getByTestId('saveSshKeyButton').click();
- await responsePromise;
+ try {
+ const sshKeyPath = path.join(SSH_KEYS_PATH, `${SSH_KEY_NAMES[sshAlgorithm]}.pub`);
+ const sshKey = await fs.readFile(sshKeyPath, 'utf8');
+
+ // Validate SSH key format
+ if (!sshKey.trim().startsWith('ssh-')) {
+ throw new Error('Invalid SSH key format');
+ }
+
+ await page.goto('user-settings/ssh');
+ await page.getByTestId('addNewSshKeyButton').click();
+ await page.getByTestId('sshKeyField').fill(sshKey);
+ const responsePromise = page.waitForResponse(`${BASE_API}/ssh-settings/public-key`);
+ await page.getByTestId('saveSshKeyButton').click();
+ await responsePromise;
+ } catch (error) {
+ throw new Error(`Failed to setup SSH credentials: ${error.message}`);
+ } finally {
+ await page.close();
+ }
}271-279:
Improve SSH validation and error handling
The SSH-specific logic needs better validation and error handling.
Apply this diff:
await programmingExerciseOverview.openCloneMenu(cloneMethod);
+if (cloneMethod === GitCloneMethod.ssh && !sshAlgorithm) {
+ throw new Error('SSH algorithm must be provided when using SSH clone method');
+}
if (cloneMethod == GitCloneMethod.ssh) {
await expect(programmingExerciseOverview.getCloneUrlButton()).toBeDisabled();
const sshKeyNotFoundAlert = page.locator('.alert', { hasText: 'To use ssh, you need to add an ssh key to your account' });
await expect(sshKeyNotFoundAlert).toBeVisible();
- await setupSSHCredentials(page.context(), sshAlgorithm);
+ try {
+ await setupSSHCredentials(page.context(), sshAlgorithm);
+ } catch (error) {
+ throw new Error(`Failed to setup SSH credentials: ${error.message}`);
+ }
await page.reload();
await programmingExerciseOverview.openCloneMenu(cloneMethod);
}Likely invalid or redundant comment.
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (1)
100-115: Add assertions to verify SSH key setupWhile the tests cover both RSA and ED25519 keys, consider adding assertions to verify:
- The SSH key was successfully added
- The clone URL is enabled after key setup
test(`Makes a git submission using SSH with ${sshAlgorithm} key`, async ({ page, programmingExerciseOverview }) => { await programmingExerciseOverview.startParticipation(course.id!, exercise.id!, studentOne); + // Verify initial state (no SSH key) + await programmingExerciseOverview.openCloneMenu(GitCloneMethod.ssh); + await expect(programmingExerciseOverview.getCloneUrlButton()).toBeDisabled(); + + // Setup SSH key and verify + await setupSSHCredentials(page.context(), sshAlgorithm); + await page.reload(); + await programmingExerciseOverview.openCloneMenu(GitCloneMethod.ssh); + await expect(programmingExerciseOverview.getCloneUrlButton()).toBeEnabled(); + await makeGitExerciseSubmission( page, programmingExerciseOverview,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docker/playwright-E2E-tests-mysql-localci.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts(3 hunks)
🔇 Additional comments (5)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (5)
8-21: LGTM: Import statements are well-organized
The new imports for SSH-related functionality are properly organized and necessary for the implementation.
84-84: LGTM: Test description is more explicit
The updated test description clearly distinguishes this test case from the new SSH-based tests.
117-119: Add error handling for SSH key cleanup
The SSH key cleanup in the afterEach hook should handle potential errors to ensure proper cleanup.
268-269: Add validation for SSH algorithm parameter
The function should validate that sshAlgorithm is provided when using SSH clone method.
301-312: Handle potential exceptions when reading SSH key file
The function should handle potential errors when reading the SSH key file.
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts
Show resolved
Hide resolved
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (1)
116-129: Enhance test descriptions with expected outcomesThe test descriptions could be more explicit about the expected outcomes to improve test readability and maintenance.
Consider updating the test description to include the expected outcome:
-test(`Makes a git submission using SSH with ${sshAlgorithm} key`, async ({ page, programmingExerciseOverview }) => { +test(`Successfully submits exercise solution via SSH using ${sshAlgorithm} key and verifies build result`, async ({ page, programmingExerciseOverview }) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts(3 hunks)
🔇 Additional comments (4)
src/test/playwright/e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts (4)
303-308: 🛠️ Refactor suggestion
Add error handling for Git operations
The Git clone operation should handle potential errors gracefully.
Apply this diff:
- let exerciseRepo;
- if (cloneMethod == GitCloneMethod.ssh) {
- exerciseRepo = await gitClient.cloneRepo(repoUrl, repoName, SSH_KEY_NAMES[sshAlgorithm]);
- } else {
- exerciseRepo = await gitClient.cloneRepo(repoUrl, repoName);
- }
+ let exerciseRepo;
+ try {
+ if (cloneMethod == GitCloneMethod.ssh) {
+ exerciseRepo = await gitClient.cloneRepo(repoUrl, repoName, SSH_KEY_NAMES[sshAlgorithm]);
+ } else {
+ exerciseRepo = await gitClient.cloneRepo(repoUrl, repoName);
+ }
+ } catch (error) {
+ console.error(`Failed to clone repository: ${error.message}`);
+ throw new Error(`Repository clone failed: ${error.message}`);
+ }Likely invalid or redundant comment.
132-134: 🛠️ Refactor suggestion
Improve error handling in the afterEach hook
The SSH key cleanup could fail silently. Consider adding error handling to ensure cleanup issues are properly logged and don't mask test failures.
Apply this diff:
test.afterEach('Delete SSH key', async ({ accountManagementAPIRequests }) => {
- await accountManagementAPIRequests.deleteSshPublicKey();
+ try {
+ await accountManagementAPIRequests.deleteSshPublicKey();
+ } catch (error) {
+ console.error('Failed to delete SSH key:', error);
+ throw error; // Re-throw to ensure test failure
+ }
});Likely invalid or redundant comment.
317-329: 🛠️ Refactor suggestion
Improve error handling and resource cleanup in SSH setup
The function needs better error handling for file operations and proper cleanup of browser resources.
Apply this diff:
async function setupSSHCredentials(context: BrowserContext, sshAlgorithm: SshEncryptionAlgorithm) {
console.log(`Setting up SSH credentials with key ${SSH_KEY_NAMES[sshAlgorithm]}`);
const page = await context.newPage();
- const sshKeyPath = path.join(SSH_KEYS_PATH, `${SSH_KEY_NAMES[sshAlgorithm]}.pub`);
- const sshKey = await fs.readFile(sshKeyPath, 'utf8');
- await page.goto('user-settings/ssh');
- await page.getByTestId('addNewSshKeyButton').click();
- await page.getByTestId('sshKeyField').fill(sshKey!);
- const responsePromise = page.waitForResponse(`${BASE_API}/ssh-settings/public-key`);
- await page.getByTestId('saveSshKeyButton').click();
- await responsePromise;
- await page.close();
+ try {
+ const sshKeyPath = path.join(SSH_KEYS_PATH, `${SSH_KEY_NAMES[sshAlgorithm]}.pub`);
+ let sshKey: string;
+ try {
+ sshKey = await fs.readFile(sshKeyPath, 'utf8');
+ if (!sshKey.trim().startsWith('ssh-')) {
+ throw new Error('Invalid SSH key format');
+ }
+ } catch (error) {
+ throw new Error(`Failed to read SSH key from ${sshKeyPath}: ${error.message}`);
+ }
+
+ await page.goto('user-settings/ssh');
+ await page.getByTestId('addNewSshKeyButton').click();
+ await page.getByTestId('sshKeyField').fill(sshKey);
+ const responsePromise = page.waitForResponse(`${BASE_API}/ssh-settings/public-key`);
+ await page.getByTestId('saveSshKeyButton').click();
+ await responsePromise;
+ } finally {
+ await page.close();
+ }
}Likely invalid or redundant comment.
287-299: 🛠️ Refactor suggestion
Add validation for SSH parameters and improve URL handling
The function should validate SSH parameters and handle repository URLs more robustly.
Apply these improvements:
+ if (cloneMethod === GitCloneMethod.ssh && !sshAlgorithm) {
+ throw new Error('SSH algorithm must be provided when using SSH clone method');
+ }
+
await programmingExerciseOverview.openCloneMenu(cloneMethod);
if (cloneMethod == GitCloneMethod.ssh) {
await expect(programmingExerciseOverview.getCloneUrlButton()).toBeDisabled();
const sshKeyNotFoundAlert = page.locator('.alert', { hasText: 'To use ssh, you need to add an ssh key to your account' });
await expect(sshKeyNotFoundAlert).toBeVisible();
await setupSSHCredentials(page.context(), sshAlgorithm);
await page.reload();
await programmingExerciseOverview.openCloneMenu(cloneMethod);
}
let repoUrl = await programmingExerciseOverview.copyCloneUrl();
if (cloneMethod == GitCloneMethod.https) {
- repoUrl = repoUrl.replace(student.username!, `${student.username!}:${student.password!}`);
+ try {
+ const url = new URL(repoUrl);
+ url.username = student.username!;
+ url.password = student.password!;
+ repoUrl = url.toString();
+ } catch (error) {
+ throw new Error(`Invalid repository URL: ${error.message}`);
+ }
}Likely invalid or redundant comment.
muradium
changed the title
Development: E2E tests for exercise participation using git via SSHDevelopment: E2E tests for Git exercise participation using SSH and Token
krusche
changed the title
Development: E2E tests for Git exercise participation using SSH and TokenDevelopment: Add e2e tests for git exercise participation using ssh and token
Checklist
General
Client
Motivation and Context
We want to test the scenario where students make programming exercise submissions by Git using SSH.
Description
Adds 2 E2E tests in Playwright about using RSA and ED25519 encrypted SSH keys for exercise submissions. For simplicity of test setup, SSH keys intented for using in Playwright have been generated inside
src/test/playwright/ssh-keysfolder. These keys are automatically copied to the ssh keys storage of the machine by Playwright global scriptglobal-script.ts, which is executed before tests are ran.Steps for Testing
Steps for running the tests:
npm install && npm run playwright:setupnpx playwright test e2e/exercise/programming/ProgrammingExerciseParticipation.spec.ts -g "Programming exercise participation using secure git"npm run playwright:opento open the Playwright UI, search for the "Programming exercise participation using secure git" test suite and run itTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
data-testidattributes to various components for improved test accessibility.Bug Fixes
Documentation
Chores
.gitignoreto include additional ignored files and organized existing entries.