Skip to content

Connect: add e2e tests for app configuration#64758

Merged
gzdunek merged 4 commits intomasterfrom
gzdunek/connect-config-file-e2e
Mar 18, 2026
Merged

Connect: add e2e tests for app configuration#64758
gzdunek merged 4 commits intomasterfrom
gzdunek/connect-config-file-e2e

Conversation

@gzdunek
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek commented Mar 18, 2026

Adds tests for the Configuration section of the test plan:

- Configuration
- [ ] Verify that clicking on More Options icon `` > Open Config File opens the `app_config.json` file in your editor.
- Verify that this works on:
- [ ] macOS
- [ ] Windows
- [ ] Linux
- Change a config property and restart the app. Verify that the change has been applied.
- [ ] Change a keyboard shortcut.
- [ ] Change `terminal.fontFamily`.
- Provide the same keyboard shortcut for two actions.
- [ ] Verify that a notification is displayed saying that a duplicate shortcut was found.
- Provide an invalid value for some property (for example, set `"keymap.tab1": "ABC"`).
- [ ] Verify that a notification is displayed saying that the property has an invalid value.
- Make a syntax error in the file (for example, set `"keymap.tab1": not a string`).
- [ ] Verify that a notification is displayed saying that the config file was not loaded correctly.
- [ ] Verify that your config changes were not overridden.

I renamed app to electronApp to avoid names like app.app (now it's app.electronApp).
I also enabled customizing the config file in tests.

Manual Test Plan

My laptop.

Test Cases

  • The new tests are passing.

@gzdunek gzdunek added the no-changelog Indicates that a PR does not require a changelog entry label Mar 18, 2026
@github-actions github-actions Bot requested review from bl-nero and r0mant March 18, 2026 12:08
@gzdunek gzdunek requested review from ravicious and ryanclark and removed request for bl-nero and r0mant March 18, 2026 12:08
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 000261f643

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread e2e/helpers/connect.ts
Comment on lines 55 to 57
await page.waitForLoadState('domcontentloaded');

const usageData = page.getByText('Anonymous usage data');
await usageData.isVisible();
const declineUsageData = page.getByRole('button', {
name: 'Decline',
exact: true,
});
await declineUsageData.click();

return { app, page, [Symbol.asyncDispose]: async () => app.close() };
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore usage-consent handling in launchApp

launchApp now returns immediately after domcontentloaded and no longer dismisses the first-run "Anonymous usage data" modal. This changes the behavior for existing callers outside the fixture path (for example e2e/scripts/open-connect.ts, which still calls launchApp(dataDir.path) directly), so on a fresh CONNECT_DATA_DIR the modal can block login() from clicking "Connect" and make the helper script fail/hang. Please keep launchApp robust for fresh profiles (or make all callers seed usageReporting.enabled=false before launching).

Useful? React with 👍 / 👎.

Comment thread e2e/helpers/connect.ts
})
);
const defaultAppConfig: Record<string, unknown> = {
'usageReporting.enabled': false,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the same setting in my other upcoming PR with tests. But since we're turning it off, I also have a separate test which explicitly removes this from the config and tests that the telemetry dialog is shown on the first launch. I'll try to rebase my stuff over your changes.

@ravicious ravicious self-requested a review March 18, 2026 12:15
Base automatically changed from r7s/connect-shell-e2e to master March 18, 2026 12:40
@gzdunek gzdunek force-pushed the gzdunek/connect-config-file-e2e branch from 000261f to b9a8a66 Compare March 18, 2026 12:47
@gzdunek gzdunek added this pull request to the merge queue Mar 18, 2026
Merged via the queue into master with commit 770e0ec Mar 18, 2026
42 checks passed
@gzdunek gzdunek deleted the gzdunek/connect-config-file-e2e branch March 18, 2026 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Indicates that a PR does not require a changelog entry size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants