Always Upload Screenshot Artifacts for Nightly Tests #8171
Conversation
WalkthroughThe changes in this pull request involve updates to two GitHub workflow files and a TypeScript file related to Puppeteer testing. In the workflow files, a conditional statement Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Puppeteer
participant BrowserStack
User->>GitHub Actions: Trigger nightly workflow
GitHub Actions->>Puppeteer: Start tests
alt Tests succeed
Puppeteer->>GitHub Actions: Upload screenshots
else Tests fail
Puppeteer->>GitHub Actions: Upload screenshots
end
GitHub Actions->>User: Notify completion
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/nightly.yml (1)
Line range hint
1-62: Consider additional workflow robustness improvements.While the current changes improve debugging capabilities, here are some suggestions to make the workflow even more robust:
- Consider adding error handling for the deployment removal/installation curl commands
- The fixed sleep times (180s) could be replaced with active polling or health checks
- The screenshot test timeout (3000s) could benefit from being a workflow input parameter
Example improvement for the deployment health check:
- name: Wait for deployment run: | - sleep 180 + max_attempts=30 + attempt=1 + while [ $attempt -le $max_attempts ]; do + if curl -s -f https://master.webknossos.xyz/health > /dev/null; then + echo "Deployment is healthy" + exit 0 + fi + echo "Waiting for deployment to be healthy (attempt $attempt/$max_attempts)..." + sleep 10 + attempt=$((attempt + 1)) + done + echo "Deployment health check failed after $max_attempts attempts" + exit 1.github/workflows/wkorg-nightly.yaml (1)
Line range hint
37-41: Consider adding a retention period for artifacts.Since these are nightly tests generating screenshots regularly, it would be good to manage storage by setting a retention period for the artifacts.
- name: Upload screenshots as artifact uses: actions/upload-artifact@v3 if: always() with: name: screenshots path: frontend/javascripts/test/screenshots-wkorg + retention-days: 14 # Adjust the number based on your debugging needsfrontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (1)
381-389: Consider improving log readability.The logging implementation is good, but the format could be more consistent. Consider removing the extra newlines and using a more structured format.
- console.log(`\nRunning chrome version ${await t.context.browser.version()}\n`); - console.log(`\nBrowserStack Session Id ${await getBrowserstackSessionId(browser)}\n`); + console.log(`Running Chrome version: ${await t.context.browser.version()}`); + console.log(`BrowserStack Session ID: ${await getBrowserstackSessionId(browser)}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
.github/workflows/nightly.yml(1 hunks).github/workflows/wkorg-nightly.yaml(1 hunks)frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts(2 hunks)
🔇 Additional comments (4)
.github/workflows/nightly.yml (2)
59-59: LGTM! Good improvement for debugging capabilities.
Adding if: always() ensures that screenshot artifacts are uploaded even when tests fail, which is crucial for debugging failing nightly tests. This change aligns perfectly with the PR objective.
Line range hint 46-57: Verify BrowserStack environment variables usage.
The workflow uses BrowserStack credentials, and the PR objectives mention improvements to BrowserStack integration. Let's verify how these credentials are used in the test code.
✅ Verification successful
Let me check the content of the file that uses BrowserStack to understand how the credentials are utilized.
BrowserStack credentials are properly configured and validated
The credentials are correctly used in the test setup:
- Environment variables are properly validated through
checkBrowserstackCredentials() - Credentials are securely passed to BrowserStack's CDP endpoint
- Variables are used as expected in the BrowserStack capabilities configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how BrowserStack credentials are used in the test code
# Expected: Find usage of BROWSERSTACK_USERNAME and BROWSERSTACK_ACCESS_KEY in test files
# Search for BrowserStack configuration in test files
echo "Searching for BrowserStack configuration..."
rg -l "BROWSERSTACK" --type ts frontend/javascripts/test/
Length of output: 219
Script:
#!/bin/bash
# Check the content of the file that uses BrowserStack
rg -A 5 -B 5 "BROWSERSTACK" frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts
Length of output: 964
.github/workflows/wkorg-nightly.yaml (1)
37-37: LGTM! Good improvement for debugging failed tests.
Adding if: always() ensures that screenshot artifacts are uploaded even when tests fail, which is crucial for debugging nightly test failures. This change aligns perfectly with the PR objective.
frontend/javascripts/test/puppeteer/dataset_rendering_helpers.ts (1)
377-381: LGTM: Test name added to BrowserStack session.
The addition of name: t.title to the BrowserStack capabilities will help identify test sessions on the BrowserStack dashboard.
| async function getBrowserstackSessionId(browser: Browser) { | ||
| const page = await browser.newPage(); | ||
| const response = (await page.evaluate( | ||
| (_) => {}, | ||
| `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}`, | ||
| )) as unknown as string; | ||
|
|
||
| const sessionDetails = await JSON.parse(response); | ||
| return sessionDetails.hashed_id; | ||
| } |
There was a problem hiding this comment.
Improve robustness of BrowserStack session ID retrieval.
The function has several areas for improvement:
- Memory leak: The created page is never closed
- Missing error handling for JSON parsing
- Unnecessary empty callback in page.evaluate
- Type safety could be improved
Consider this improved implementation:
async function getBrowserstackSessionId(browser: Browser) {
const page = await browser.newPage();
- const response = (await page.evaluate(
- (_) => {},
- `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}`,
- )) as unknown as string;
+ try {
+ const response = await page.evaluate(
+ `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}`
+ ) as string;
+
+ const sessionDetails = JSON.parse(response) as { hashed_id: string };
+ return sessionDetails.hashed_id;
+ } catch (error) {
+ console.error('Failed to retrieve BrowserStack session ID:', error);
+ throw error;
+ } finally {
+ await page.close();
+ }
- const sessionDetails = await JSON.parse(response);
- return sessionDetails.hashed_id;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function getBrowserstackSessionId(browser: Browser) { | |
| const page = await browser.newPage(); | |
| const response = (await page.evaluate( | |
| (_) => {}, | |
| `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}`, | |
| )) as unknown as string; | |
| const sessionDetails = await JSON.parse(response); | |
| return sessionDetails.hashed_id; | |
| } | |
| async function getBrowserstackSessionId(browser: Browser) { | |
| const page = await browser.newPage(); | |
| try { | |
| const response = await page.evaluate( | |
| `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}` | |
| ) as string; | |
| const sessionDetails = JSON.parse(response) as { hashed_id: string }; | |
| return sessionDetails.hashed_id; | |
| } catch (error) { | |
| console.error('Failed to retrieve BrowserStack session ID:', error); | |
| throw error; | |
| } finally { | |
| await page.close(); | |
| } | |
| } |
| @@ -55,7 +55,8 @@ jobs: | |||
| BROWSERSTACK_ACCESS_KEY : ${{ secrets.BROWSERSTACK_ACCESS_KEY }} | |||
|
|
|||
| - name: Upload screenshots as artifact | |||
| uses: actions/upload-artifact@v3 | |||
| uses: actions/upload-artifact@v4 | |||
There was a problem hiding this comment.
Updated to v4 since v3 is EOL at the end of the year.
| const page = await browser.newPage(); | ||
| const response = (await page.evaluate( | ||
| (_) => {}, | ||
| `browserstack_executor: ${JSON.stringify({ action: "getSessionDetails" })}`, |
There was a problem hiding this comment.
|
Seems to work. See this action run which includes BrowserStack Ids and the BrowserStack UI showing names for each invocation: https://github.com/scalableminds/webknossos/actions/runs/11701122812/job/32586545549 
|
Changes:
Steps to test:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation