fix: make sure configs are correctly inherited

sheremet-va · sheremet-va · commit 227094e2a23a · 2024-12-04T18:03:54.000+01:00
diff --git a/packages/vitest/src/node/core.ts b/packages/vitest/src/node/core.ts
@@ -290,7 +290,7 @@ export class Vitest {
     this._workspaceConfigPath = workspaceConfigPath
 
     if (!workspaceConfigPath) {
-      return resolveBrowserWorkspace(this, [this._createRootProject()])
+      return resolveBrowserWorkspace(this, new Set(), [this._createRootProject()])
     }
 
     const workspaceModule = await this.runner.executeFile(workspaceConfigPath) as {
diff --git a/packages/vitest/src/node/project.ts b/packages/vitest/src/node/project.ts
@@ -158,6 +158,12 @@ export class TestProject {
     if (!this._vite) {
       throw new Error('The server was not set. It means that `project.vite` was called before the Vite server was established.')
     }
+    // checking it once should be enough
+    Object.defineProperty(this, 'vite', {
+      configurable: true,
+      writable: true,
+      value: this._vite,
+    })
     return this._vite
   }
 
@@ -168,6 +174,12 @@ export class TestProject {
     if (!this._config) {
       throw new Error('The config was not set. It means that `project.config` was called before the Vite server was established.')
     }
+    // checking it once should be enough
+    Object.defineProperty(this, 'config', {
+      configurable: true,
+      writable: true,
+      value: this._config,
+    })
     return this._config
   }
 
diff --git a/packages/vitest/src/node/types/browser.ts b/packages/vitest/src/node/types/browser.ts
@@ -66,8 +66,10 @@ type UnsupportedProperties =
   | 'server'
   | 'benchmark'
 
-// TODO: document all options
-export interface BrowserConfig extends BrowserProviderOptions, Omit<ProjectConfig, UnsupportedProperties>, Pick<BrowserConfigOptions, 'locators' | 'viewport' | 'testerHtmlPath' | 'screenshotDirectory' | 'screenshotFailures'> {
+export interface BrowserConfig
+  extends BrowserProviderOptions,
+  Omit<ProjectConfig, UnsupportedProperties>,
+  Pick<BrowserConfigOptions, 'locators' | 'viewport' | 'testerHtmlPath' | 'screenshotDirectory' | 'screenshotFailures'> {
   browser: string
 }
 
diff --git a/packages/vitest/src/node/workspace/resolveWorkspace.ts b/packages/vitest/src/node/workspace/resolveWorkspace.ts
@@ -1,5 +1,5 @@
 import type { Vitest } from '../core'
-import type { ResolvedConfig, TestProjectConfiguration, UserConfig, UserWorkspaceConfig } from '../types/config'
+import type { BrowserConfig, ResolvedConfig, TestProjectConfiguration, UserConfig, UserWorkspaceConfig } from '../types/config'
 import { existsSync, promises as fs } from 'node:fs'
 import os from 'node:os'
 import { limitConcurrency } from '@vitest/runner/utils'
@@ -97,7 +97,7 @@ export async function resolveWorkspace(
 
   // pretty rare case - the glob didn't match anything and there are no inline configs
   if (!projectPromises.length) {
-    return resolveBrowserWorkspace(vitest, [vitest._createRootProject()])
+    return resolveBrowserWorkspace(vitest, new Set(), [vitest._createRootProject()])
   }
 
   const resolvedProjects = await Promise.all(projectPromises)
@@ -127,76 +127,101 @@ export async function resolveWorkspace(
     names.add(name)
   }
 
-  return resolveBrowserWorkspace(vitest, resolvedProjects)
+  return resolveBrowserWorkspace(vitest, names, resolvedProjects)
 }
 
 export function resolveBrowserWorkspace(
   vitest: Vitest,
+  names: Set<string>,
   resolvedProjects: TestProject[],
 ) {
+  const newConfigs: [project: TestProject, config: ResolvedConfig][] = []
+
   resolvedProjects.forEach((project) => {
     const configs = project.config.browser.configs
     if (!project.config.browser.enabled || !configs || configs.length === 0) {
       return
     }
     const [firstConfig, ...restConfigs] = configs
+    const originalName = project.config.name
 
-    project.config.name ||= project.config.name
-      ? `${project.config.name} (${firstConfig.browser})`
+    const newName = originalName
+      ? `${originalName} (${firstConfig.browser})`
       : firstConfig.browser
+    if (names.has(newName)) {
+      throw new Error(`Cannot redefine the project name for a nameless project. The project name "${firstConfig.browser}" was already defined. All projects in a workspace should have unique names. Make sure your configuration is correct.`)
+    }
+    names.add(newName)
 
     if (project.config.browser.providerOptions) {
       vitest.logger.warn(
-        withLabel('yellow', 'Vitest', `"providerOptions"${project.config.name ? ` in "${project.config.name}" project` : ''} is ignored because it's overriden by the configs. To hide this warning, remove the "providerOptions" property from the browser configuration.`),
+        withLabel('yellow', 'Vitest', `"providerOptions"${originalName ? ` in "${originalName}" project` : ''} is ignored because it's overriden by the configs. To hide this warning, remove the "providerOptions" property from the browser configuration.`),
       )
     }
 
-    project.config.browser.name = firstConfig.browser
-    project.config.browser.providerOptions = firstConfig
-
-    restConfigs.forEach(({ browser, ...capability }) => {
-      // TODO: cover with tests
-      // browser-only options
-      const {
-        locators,
-        viewport,
-        testerHtmlPath,
-        screenshotDirectory,
-        screenshotFailures,
-        // @ts-expect-error remove just in case
-        browser: _browser,
-        // TODO: need a lot of tests
-        ...overrideConfig
-      } = capability
-      const currentConfig = project.config.browser
-      const clonedConfig = mergeConfig<any, any>({
-        ...project.config,
-        name: project.config.name ? `${project.config} (${browser})` : browser,
-        browser: {
-          ...project.config.browser,
-          locators: locators
-            ? {
-                testIdAttribute: locators.testIdAttribute ?? currentConfig.locators.testIdAttribute,
-              }
-            : project.config.browser.locators,
-          viewport: viewport ?? currentConfig.viewport,
-          testerHtmlPath: testerHtmlPath ?? currentConfig.testerHtmlPath,
-          screenshotDirectory: screenshotDirectory ?? currentConfig.screenshotDirectory,
-          screenshotFailures: screenshotFailures ?? currentConfig.screenshotFailures,
-          name: browser,
-          providerOptions: capability,
-          configs: undefined, // projects cannot spawn more configs
-        },
-        // TODO: should resolve, not merge/override
-      } satisfies ResolvedConfig, overrideConfig) as ResolvedConfig
-      const clone = TestProject._cloneBrowserProject(project, clonedConfig)
-
-      resolvedProjects.push(clone)
+    restConfigs.forEach((config) => {
+      const browser = config.browser
+      const name = config.name
+      const newName = name || (originalName ? `${originalName} (${browser})` : browser)
+      if (names.has(newName)) {
+        throw new Error(
+          [
+            `Cannot define a nested project for a ${browser} browser. The project name "${newName}" was already defined. `,
+            'If you have multiple configs for the same browser, make sure to define a custom "name". ',
+            'All projects in a workspace should have unique names. Make sure your configuration is correct.',
+          ].join(''),
+        )
+      }
+      names.add(newName)
+      const clonedConfig = cloneConfig(project, config)
+      clonedConfig.name = newName
+      newConfigs.push([project, clonedConfig])
     })
+
+    Object.assign(project.config, cloneConfig(project, firstConfig))
+    project.config.name = newName
+  })
+  newConfigs.forEach(([project, clonedConfig]) => {
+    const clone = TestProject._cloneBrowserProject(project, clonedConfig)
+    resolvedProjects.push(clone)
   })
   return resolvedProjects
 }
 
+function cloneConfig(project: TestProject, { browser, ...config }: BrowserConfig) {
+  const {
+    locators,
+    viewport,
+    testerHtmlPath,
+    screenshotDirectory,
+    screenshotFailures,
+    // @ts-expect-error remove just in case
+    browser: _browser,
+    name,
+    ...overrideConfig
+  } = config
+  const currentConfig = project.config.browser
+  return mergeConfig<any, any>({
+    ...project.config,
+    browser: {
+      ...project.config.browser,
+      locators: locators
+        ? {
+            testIdAttribute: locators.testIdAttribute ?? currentConfig.locators.testIdAttribute,
+          }
+        : project.config.browser.locators,
+      viewport: viewport ?? currentConfig.viewport,
+      testerHtmlPath: testerHtmlPath ?? currentConfig.testerHtmlPath,
+      screenshotDirectory: screenshotDirectory ?? currentConfig.screenshotDirectory,
+      screenshotFailures: screenshotFailures ?? currentConfig.screenshotFailures,
+      name: browser,
+      providerOptions: config,
+      configs: undefined, // projects cannot spawn more configs
+    },
+    // TODO: should resolve, not merge/override
+  } satisfies ResolvedConfig, overrideConfig) as ResolvedConfig
+}
+
 async function resolveTestProjectConfigs(
   vitest: Vitest,
   workspaceConfigPath: string | undefined,
diff --git a/packages/vitest/src/public/node.ts b/packages/vitest/src/public/node.ts
@@ -5,7 +5,7 @@ import { TestModule as _TestFile } from '../node/reporters/reported-tasks'
 export { parseCLI } from '../node/cli/cac'
 export { startVitest } from '../node/cli/cli-api'
 export { resolveApiServerConfig, resolveConfig } from '../node/config/resolveConfig'
-export type { Vitest } from '../node/core'
+export type { Vitest, VitestOptions } from '../node/core'
 export { createVitest } from '../node/create'
 export { GitNotFoundError, FilesNotFoundError as TestsNotFoundError } from '../node/errors'
 export type { GlobalSetupContext } from '../node/globalSetup'
diff --git a/test/config/test/browser-configs.test.ts b/test/config/test/browser-configs.test.ts
@@ -0,0 +1,141 @@
+import type { ViteUserConfig } from 'vitest/config'
+import type { UserConfig, VitestOptions } from 'vitest/node'
+import { expect, test } from 'vitest'
+import { createVitest } from 'vitest/node'
+
+async function vitest(cliOptions: UserConfig, configValue: UserConfig = {}, viteConfig: ViteUserConfig = {}, vitestOptions: VitestOptions = {}) {
+  return await createVitest('test', { ...cliOptions, watch: false }, { ...viteConfig, test: configValue as any }, vitestOptions)
+}
+
+test('assignes names as browsers', async () => {
+  const { projects } = await vitest({
+    browser: {
+      enabled: true,
+      configs: [
+        { browser: 'chromium' },
+        { browser: 'firefox' },
+        { browser: 'webkit' },
+      ],
+    },
+  })
+  expect(projects.map(p => p.name)).toEqual([
+    'chromium',
+    'firefox',
+    'webkit',
+  ])
+})
+
+test('assignes names as browsers in a custom project', async () => {
+  const { projects } = await vitest({
+    workspace: [
+      {
+        test: {
+          name: 'custom',
+          browser: {
+            enabled: true,
+            configs: [
+              { browser: 'chromium' },
+              { browser: 'firefox' },
+              { browser: 'webkit' },
+              { browser: 'webkit', name: 'hello-world' },
+            ],
+          },
+        },
+      },
+    ],
+  })
+  expect(projects.map(p => p.name)).toEqual([
+    'custom (chromium)',
+    'custom (firefox)',
+    'custom (webkit)',
+    'hello-world',
+  ])
+})
+
+test.only('inherits browser options', async () => {
+  const { projects } = await vitest({
+    setupFiles: ['/test/setup.ts'],
+    provide: {
+      browser: true,
+    },
+    browser: {
+      enabled: true,
+      headless: true,
+      screenshotFailures: false,
+      testerHtmlPath: '/custom-path.html',
+      screenshotDirectory: '/custom-directory',
+      fileParallelism: false,
+      viewport: {
+        width: 300,
+        height: 900,
+      },
+      locators: {
+        testIdAttribute: 'data-tid',
+      },
+      configs: [
+        {
+          browser: 'chromium',
+          screenshotFailures: true,
+        },
+        {
+          browser: 'firefox',
+          screenshotFailures: true,
+          locators: {
+            testIdAttribute: 'data-custom',
+          },
+          viewport: {
+            width: 900,
+            height: 300,
+          },
+          testerHtmlPath: '/custom-overriden-path.html',
+          screenshotDirectory: '/custom-overriden-directory',
+        },
+      ],
+    },
+  })
+  expect(projects.map(p => p.config)).toMatchObject([
+    {
+      name: 'chromium',
+      setupFiles: ['/test/setup.ts'],
+      provide: {
+        browser: true,
+      },
+      browser: {
+        enabled: true,
+        headless: true,
+        screenshotFailures: true,
+        screenshotDirectory: '/custom-directory',
+        viewport: {
+          width: 300,
+          height: 900,
+        },
+        locators: {
+          testIdAttribute: 'data-tid',
+        },
+        fileParallelism: false,
+        testerHtmlPath: '/custom-path.html',
+      },
+    },
+    {
+      name: 'firefox',
+      setupFiles: ['/test/setup.ts'],
+      provide: {
+        browser: true,
+      },
+      browser: {
+        enabled: true,
+        headless: true,
+        screenshotFailures: true,
+        viewport: {
+          width: 900,
+          height: 300,
+        },
+        screenshotDirectory: '/custom-overriden-directory',
+        locators: {
+          testIdAttribute: 'data-custom',
+        },
+        testerHtmlPath: '/custom-overriden-path.html',
+      },
+    },
+  ])
+})
diff --git a/test/config/test/failures.test.ts b/test/config/test/failures.test.ts
@@ -319,3 +319,33 @@ test('browser.name filteres all browser.configs are required', async () => {
   })
   expect(stderr).toMatch('"browser.configs" was set in the config, but the array is empty. Define at least one browser config. The "browser.name" was set to "chromium" which filtered all configs (firefox). Did you mean to use another name?')
 })
+
+test('browser.configs throws an error if no custom name is provided', async () => {
+  const { stderr } = await runVitest({
+    browser: {
+      enabled: true,
+      provider: 'playwright',
+      configs: [
+        { browser: 'firefox' },
+        { browser: 'firefox' },
+      ],
+    },
+  })
+  expect(stderr).toMatch('Cannot define a nested project for a firefox browser. The project name "firefox" was already defined. If you have multiple configs for the same browser, make sure to define a custom "name". All projects in a workspace should have unique names. Make sure your configuration is correct.')
+})
+
+test('browser.configs throws an error if no custom name is provided, but the config name is inherited', async () => {
+  const { stderr } = await runVitest({
+    name: 'custom',
+    browser: {
+      enabled: true,
+      provider: 'playwright',
+      configs: [
+        { browser: 'firefox' },
+        { browser: 'firefox' },
+      ],
+    },
+  })
+  expect(stderr).toMatch('Cannot define a nested project for a firefox browser. The project name "custom (firefox)" was already defined. If you have multiple configs for the same browser, make sure to define a custom "name". All projects in a workspace should have unique names. Make sure your configuration is correct.')
+})
+

Original file line number	Diff line number	Diff line change
`@@ -290,7 +290,7 @@ export class Vitest {`
`290`	`290`	`this._workspaceConfigPath = workspaceConfigPath`
`291`	`291`
`292`	`292`	`if (!workspaceConfigPath) {`
`293`		`- return resolveBrowserWorkspace(this, [this._createRootProject()])`
	`293`	`+ return resolveBrowserWorkspace(this, new Set(), [this._createRootProject()])`
`294`	`294`	`}`
`295`	`295`
`296`	`296`	`const workspaceModule = await this.runner.executeFile(workspaceConfigPath) as {`
Original file line number	Diff line number	Diff line change
`@@ -158,6 +158,12 @@ export class TestProject {`
`158`	`158`	`if (!this._vite) {`
`159`	`159`	throw new Error('The server was not set. It means that `project.vite` was called before the Vite server was established.')
`160`	`160`	`}`
	`161`	`+ // checking it once should be enough`
	`162`	`+ Object.defineProperty(this, 'vite', {`
	`163`	`+ configurable: true,`
	`164`	`+ writable: true,`
	`165`	`+ value: this._vite,`
	`166`	`+ })`
`161`	`167`	`return this._vite`
`162`	`168`	`}`
`163`	`169`
`@@ -168,6 +174,12 @@ export class TestProject {`
`168`	`174`	`if (!this._config) {`
`169`	`175`	throw new Error('The config was not set. It means that `project.config` was called before the Vite server was established.')
`170`	`176`	`}`
	`177`	`+ // checking it once should be enough`
	`178`	`+ Object.defineProperty(this, 'config', {`
	`179`	`+ configurable: true,`
	`180`	`+ writable: true,`
	`181`	`+ value: this._config,`
	`182`	`+ })`
`171`	`183`	`return this._config`
`172`	`184`	`}`
`173`	`185`