chore: directly update settings.json during ui testing #2398
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors UI test settings management to directly update settings.json instead of manipulating the VS Code settings UI. The change introduces a new ensureSettings() function that writes settings directly to the filesystem, replacing the previous pattern of opening the settings editor and programmatically changing values through the UI.
Key changes:
- Introduced
ensureSettings()function that directly modifiessettings.jsonwith idempotent file writes - Replaced all UI-based settings updates with direct file system updates across test files
- Added
TEST_RESOURCESenvironment variable to centralize test resource paths
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ui/uiTestHelper.ts | Added new ensureSettings() function with helper functions deepEqual() and sortObjectKeys() for direct settings file manipulation |
| test/ui/terminal.test.ts | Replaced openSettings() and updateSettings() calls with ensureSettings(), removed unused imports and variables |
| test/ui/ls/statusBarAndExplorerView.test.ts | Replaced UI-based settings updates with ensureSettings() |
| test/ui/ls/auth.test.ts | Consolidated multiple settings updates into single ensureSettings() call, removed unused workbench variable |
| test/ui/ls/activeBar.test.ts | Replaced UI-based settings updates with ensureSettings() |
| test/ui/ls/LLMProviderRoleGen.test.ts | Consolidated multiple sequential settings updates into single ensureSettings() call |
| test/ui/ls/LLMProviderPlaybookGen.test.ts | Consolidated multiple sequential settings updates into single ensureSettings() call |
| test/ui/mockLightspeedServer/openUrl.ts | Added TEST_RESOURCES variable with default value for consistent path handling |
| tools/test-launcher.sh | Updated to use TEST_RESOURCES variable instead of hardcoded paths |
| knip.ts | Added .ansible directory to ignore list |
| .env | Defined TEST_RESOURCES environment variable with default value |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sed -i.bak "s,https://c.ai.ansible.redhat.com,$TEST_LIGHTSPEED_URL," out/settings.json | ||
| fi | ||
| rm -rf out/test-resources/settings/ >/dev/null | ||
| rm -rf "${TEST_RESOURCES}/settings/" >/dev/null |
There was a problem hiding this comment.
The TEST_RESOURCES variable is used here but it is not defined in this script. Since the script uses 'set -euo pipefail', this will cause the script to fail if TEST_RESOURCES is not set in the environment. The variable should be defined at the top of the script with a default value, similar to how other variables like CODE_VERSION are defined.
| log notice "Testing ${test_file}" | ||
| log notice "Cleaning existing User settings..." | ||
| rm -rfv ./out/test-resources/settings/User/ > /dev/null | ||
| rm -rfv "./${TEST_RESOURCES}/settings/User/" > /dev/null |
There was a problem hiding this comment.
The TEST_RESOURCES variable is used here but it is not defined in this script. Since the script uses 'set -euo pipefail', this will cause the script to fail if TEST_RESOURCES is not set in the environment. The variable should be defined at the top of the script with a default value.
| timeout --kill-after=15 --preserve-status 150s npm exec -- extest run-tests \ | ||
| --mocha_config test/ui/.mocharc.js \ | ||
| -s out/test-resources \ | ||
| -s "${TEST_RESOURCES}" \ |
There was a problem hiding this comment.
The TEST_RESOURCES variable is used here but it is not defined in this script. Since the script uses 'set -euo pipefail', this will cause the script to fail if TEST_RESOURCES is not set in the environment. The variable should be defined at the top of the script with a default value.
| let existingSettings: Record<string, any> = {}; | ||
| if (fs.existsSync(settingsPath)) { | ||
| const settingsContent = fs.readFileSync(settingsPath, "utf-8"); | ||
| existingSettings = JSON.parse(settingsContent); |
There was a problem hiding this comment.
JSON parsing should be wrapped in a try-catch block to handle cases where the settings.json file exists but contains invalid JSON. If the file is corrupted, the function will crash with an unhandled exception instead of gracefully handling the error or recreating the file.
| existingSettings = JSON.parse(settingsContent); | |
| try { | |
| existingSettings = JSON.parse(settingsContent); | |
| } catch { | |
| // If settings.json is invalid or corrupted, start from a clean slate | |
| existingSettings = {}; | |
| } |
| * Updates the settings.json file directly by merging provided settings | ||
| * with existing settings and writing to $TEST_RESOURCES/settings/User/settings.json | ||
| * This function is idempotent - it only writes to the file if the content has changed. | ||
| * @param settings - Object containing settings to override (e.g., \{ "ansible.lightspeed.enabled": true \}) |
There was a problem hiding this comment.
The JSDoc example contains escaped curly braces ({ and }) which should be unescaped. In JSDoc comments, curly braces don't need to be escaped in code examples within the @param tag description.
| * @param settings - Object containing settings to override (e.g., \{ "ansible.lightspeed.enabled": true \}) | |
| * @param settings - Object containing settings to override (e.g., { "ansible.lightspeed.enabled": true }) |
1 participant
No description provided.