feat: on new-opened models, enable all projects #575
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/testModel.ts:121
- Ensure that there are tests verifying that all projects are enabled when no settings are found. The previous logic had a special case for the first project, which is now removed. Verify that this change doesn't break any existing functionality.
project.isEnabled = true;
| model._loadModelIfNeeded(configSettings).then(() => { | ||
| this._didUpdate.fire(); | ||
| if (userGesture) | ||
| this._saveSettings(); |
There was a problem hiding this comment.
Before, we were saving settings immediately before reading them in this._configSettings. This breaks the "no settings yet" path in _loadModelIfNeeded, and broke tests intrack-config.spec.ts. By moving the save settings call after the _loadModel, we keep the "no settings yet" path working.
There was a problem hiding this comment.
How is the following case handled:
- I am like 90% of our users
- I created a playwright project a year ago, I only have one config, 'chromium' was checked for me automatically
- For the last year I was pressing "Run" button and I was getting all my tests run against Chromium
- I don't expect this to change when I update extension.
There was a problem hiding this comment.
Well when I wrote this I tried to change it a way that existing users don't have any changes. Migrating settings is wicked though, so let me try writing a test to verify this.
There was a problem hiding this comment.
Oh no :/ If the user never made a gesture that invokes _saveSettings, then their workspace settings are undefined, making them indistinguishable from users opening a new workspace. Not sure why, but I somehow assumed that we'd save settings immediately on opening. I'll have to rethink this.
|
After talking to Max and Dima about it, it looks like we currently don't have any markers to distinguish existing users from new users. If we want to prevent breaking changes to existing users' workflows, while also changing default settings, we'll need to add a marker first. I'll close this PR for now and work on shipping that marker first. |
|
Here's a PR to add that marker: #577 After rolling that out, we'll wait for a couple of weeks for folks to update + activate that code in their Playwright workspace. Those workspaces will then be detectable as existing workspaces, so they don't get the new project defaults. |
Closes microsoft/playwright#33865.