playwright icon indicating copy to clipboard operation
playwright copied to clipboard

[BUG] mergeTests results in incorrect type in cases of name conflict

Open alexandersorokin opened this issue 2 years ago • 4 comments

System info

  • Playwright Version: v1.40
  • Operating System: All
  • Browser: All

Source code

import {mergeTests, test as base} from '@playwright/test';

const fix1Base = base.extend<{
	readonly commonName: string;
}>({
	commonName: async ({}, use) => {
		await use('hello!!!');
	},
});

const fix1Child = fix1Base.extend<{
	readonly data1: string;
}>({
	data1: async ({commonName}, use) => {
		await use(commonName);
	},
});

const fix2Base = base.extend<{
	readonly commonName: number;
}>({
	commonName: async ({}, use) => {
		await use(222222);
	},
});

const fix2Child = fix2Base.extend<{
	readonly data2: number;
}>({
	data2: async ({commonName}, use) => {
		await use(commonName);
	},
});

const test = mergeTests(fix1Child, fix2Child);
test('example', async ({commonName, data1, data2}): Promise<void> => {
	console.info(commonName);
	console.info(data1);
	console.info(data2);
});

Expected

  1. An error or the following output:
222222 or hello!!!
hello!!!
222222
  1. fix1Child receives commonName of string type as it is declared in dependency.

  2. commonName in test has string or number type.

Actual

  1. Output:
222222
222222
222222
  1. fix1Child receives commonName of unrelated number type.

  2. commonName in test has never type.

The main issue is passing an unexpected number to fix1Child instead of string. So when mergeTests is used, fixtures can't rely on the type of declared dependencies. In large codebases tests can fail unexpectedly if two independent fixture hierarchies have fields with the same name and different types, and then those hierarchies merged.

P.S. I see no issue with overriding fixture values in a single hierarchy because intermediate fixtures 'know' about all ancestors, and all descendants preserve type information.

alexandersorokin avatar Jan 25 '24 20:01 alexandersorokin

This is working as intended, the fixtures are resolved at run time and there is exactly one fixture called commonName. The implementation is always the latest one from the merged tests in the list that have a fixture with such name.

yury-s avatar Jan 25 '24 22:01 yury-s

  1. commonName in test has string or number type.

The inferred type is intersection of string and number which is never. So "commonName in test has never type." is expected behavior.

yury-s avatar Jan 25 '24 22:01 yury-s

I have read the code. So I understand why it works that way and why solving the issue is not such an easy task (it seems that all fixtures are flattened into a single list before resolving, and the fixture graph created by extend/mergeTests is not preserved). That behavior was perfectly fine before mergeTests was introduced, as the extend function alone can only produce a single linked chain of fixtures (there were no multiple inheritance).

Nonetheless, after mergeTests introduction it can result in unexpected bugs that were not possible before:

  1. Parameter of never type can be passed to a function that requires boolean because type info is lost.
  2. Changing fixture in one part of the codebase may result in failures in other unrelated parts of the codebase. In the example from docs both dbTest and a11yTest can be stored in separate npm packages. They can both introduce a fixture named config at some point of their lifetime. From the perspective of both packages introduction of internal config fixture is a non-breaking change, but if they both do so, their consumers will be broken without ability to fix it.

So "commonName in test has never type." is expected behavior.

I am not sure about that. If the last fixture on the list wins, then the type should also be the last one (and not the intersection).

alexandersorokin avatar Jan 26 '24 09:01 alexandersorokin

We don's support fixture name collisions, you should make sure the fixtures use unique names. We could throw in the case when there are conflicting fixtures.

yury-s avatar Jan 29 '24 18:01 yury-s