Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: on new-opened models, enable all projects #575

Closed
wants to merge 7 commits into from
Closed

feat: on new-opened models, enable all projects #575

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8f5d0d4
on newly opened models, enable all projects
Skn0tt Dec 16, 2024
527f952
on previously-opened projects, only enable previously-enabled projects
Skn0tt Dec 16, 2024
7574a8e
add tests for behaviour
Skn0tt Dec 16, 2024
be8ffba
update test name
Skn0tt Dec 16, 2024
18b4abc
save workspace settings after loading models
Skn0tt Dec 16, 2024
2d648ce
fix a bunch of tests
Skn0tt Dec 16, 2024
b4b2fe6
move _saveSettings above didUpdate
Skn0tt Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/testModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,16 @@ export class TestModel extends DisposableBase {
return;
await this._listFiles();
if (configSettings) {
let firstProject = true;
for (const project of this.projects()) {
const projectSettings = configSettings.projects.find(p => p.name === project.name);
if (projectSettings)
project.isEnabled = projectSettings.enabled;
else if (firstProject)
project.isEnabled = true;
firstProject = false;
else
project.isEnabled = false;
}
} else {
if (this.projects().length)
this.projects()[0].isEnabled = true;
for (const project of this.projects())
project.isEnabled = true;
}
}

Expand Down Expand Up @@ -715,11 +713,13 @@ export class TestModelCollection extends DisposableBase {
if (model.isEnabled === enabled)
return;
model.isEnabled = enabled;
if (userGesture)
this._saveSettings();
model.reset();
const configSettings = this._configSettings(model.config);
model._loadModelIfNeeded(configSettings).then(() => this._didUpdate.fire());
model._loadModelIfNeeded(configSettings).then(() => {
this._didUpdate.fire();
if (userGesture)
this._saveSettings();
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Skn0tt Skn0tt Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

});
}

setProjectEnabled(configFile: string, name: string, enabled: boolean) {
Expand Down
3 changes: 2 additions & 1 deletion tests/list-files.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ test('should switch between multiple projects with filter', async ({ activate })
await expect(testController).toHaveTestTree(`
- tests
- test1.spec.ts
- test2.spec.ts
`);

await expect(vscode).toHaveExecLog(`
Expand All @@ -322,7 +323,7 @@ test('should switch between multiple projects with filter', async ({ activate })
await expect(vscode).toHaveProjectTree(`
config: playwright.config.js
[x] project 1
[ ] project 2
[x] project 2
`);

await enableProjects(vscode, ['project 2']);
Expand Down
1 change: 1 addition & 0 deletions tests/list-tests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,7 @@ test('should show project-specific tests', async ({ activate }, testInfo) => {
`
});

await enableProjects(vscode, ['chromium']);
await expect(testController).toHaveTestTree(`
- test.spec.ts
`);
Expand Down
5 changes: 3 additions & 2 deletions tests/project-tree.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ test('should switch between configs', async ({ activate }) => {
await expect(vscode).toHaveProjectTree(`
config: tests1/playwright.config.js
[x] projectOne
[ ] projectTwo
[x] projectTwo
`);

await expect(vscode).toHaveExecLog(`
Expand All @@ -52,7 +52,7 @@ test('should switch between configs', async ({ activate }) => {
await expect(vscode).toHaveProjectTree(`
config: tests2/playwright.config.js
[x] projectThree
[ ] projectFour
[x] projectFour
`);

await expect(testController).toHaveTestTree(`
Expand Down Expand Up @@ -87,6 +87,7 @@ test('should switch between projects', async ({ activate }) => {
`,
});

await enableProjects(vscode, ['projectOne']);
await expect(testController).toHaveTestTree(`
- tests1
- test.spec.ts
Expand Down
5 changes: 3 additions & 2 deletions tests/run-tests.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ test('should group projects by config', async ({ activate }) => {
await expect(vscode).toHaveProjectTree(`
config: tests2/playwright.config.js
[x] projectOne
[ ] projectTwo
[x] projectTwo
`);

await enableProjects(vscode, ['projectOne', 'projectTwo']);
Expand Down Expand Up @@ -954,8 +954,9 @@ test('should filter selected project', async ({ activate }) => {
`,
});

await enableProjects(vscode, ['project 1']);

const testItems = testController.findTestItems(/test.spec/);
expect(testItems.length).toBe(1);
const testRun = await testController.run(testItems);
expect(testRun.renderLog()).toBe(`
tests1 > test.spec.ts > one [2:0]
Expand Down
81 changes: 81 additions & 0 deletions tests/settings.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,84 @@ test('should reload when playwright.env changes', async ({ activate }) => {
expect(output).toContain(`foo=foo-value`);
expect(output).toContain(`bar={"prop":"bar-value"}`);
});

test('should sync project enabled state to workspace settings', async ({ activate }) => {
const { vscode } = await activate({
'playwright.config.ts': `
import { defineConfig } from '@playwright/test';
export default defineConfig({
projects: [
{ name: 'foo', testMatch: 'foo.ts' },
{ name: 'bar', testMatch: 'bar.ts' },
]
});
`,
});

const webView = vscode.webViews.get('pw.extension.settingsView')!;

await expect(webView.locator('body')).toMatchAriaSnapshot(`
- text: PROJECTS
- checkbox "foo" [checked]
- checkbox "bar" [checked]
`);
expect(vscode.context.workspaceState.get('pw.workspace-settings')).toBeUndefined();

await webView.getByRole('checkbox', { name: 'bar' }).uncheck();

await expect(webView.locator('body')).toMatchAriaSnapshot(`
- checkbox "foo" [checked]
- checkbox "bar" [checked=false]
`);

console.log(JSON.stringify(vscode.context.workspaceState.get('pw.workspace-settings')));

expect(vscode.context.workspaceState.get('pw.workspace-settings')).toEqual(expect.objectContaining({
configs: [
expect.objectContaining({
enabled: true,
projects: [
expect.objectContaining({ name: 'foo', enabled: true }),
expect.objectContaining({ name: 'bar', enabled: false }),
]
})
]
}));
});

test('should read project enabled state from workspace settings', async ({ vscode, activate }) => {
vscode.context.workspaceState.update('pw.workspace-settings', {
configs: [
{
relativeConfigFile: 'playwright.config.ts',
selected: true,
enabled: true,
projects: [
{ name: 'foo', enabled: true },
{ name: 'bar', enabled: false }
]
}
]
});

await activate({
'playwright.config.ts': `
import { defineConfig } from '@playwright/test';
export default defineConfig({
projects: [
{ name: 'foo', testMatch: 'foo.ts' },
{ name: 'bar', testMatch: 'bar.ts' },
{ name: 'baz', testMatch: 'baz.ts' },
]
});
`,
});

const webView = vscode.webViews.get('pw.extension.settingsView')!;
await expect(webView.locator('body')).toMatchAriaSnapshot(`
- text: PROJECTS
- checkbox "foo" [checked]
- checkbox "bar" [checked=false]
- checkbox "baz" [checked=false]
`);
});
Loading