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.

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

[Feature] Setup default timeout to expect.toPass assertion #23937

Closed
danielsobon opened this issue Jun 28, 2023 · 12 comments · Fixed by #28231
Closed

[Feature] Setup default timeout to expect.toPass assertion #23937

danielsobon opened this issue Jun 28, 2023 · 12 comments · Fixed by #28231

Comments

@danielsobon
Copy link

Hey, is it possible to setup default timeout for expect.toPass assertions?
In case of usage expect.toPass without defining timeout in assertion the step will wait till the test timeout is exceed.

test('Expect toPass @desktop', async () => {
    await expect(() => {
      expect(1).toBe(2);
    }).toPass();
  });

Can we use here default expect config value?
config = { expect: { timeout: 10000 } }
Or add the new parameter into PlaywrightTestConfig

@mxschmitt
Copy link
Member

Can we use here default expect config value?

This was intentionally made, we can add a new parameter to toPass: { timeout?: number } to the config and expect.configure as part of this feature request.

@danielsobon
Copy link
Author

looks great, thx @mxschmitt

@gskierk
Copy link

gskierk commented Jul 10, 2023

It's super confusing right now to me... It's natural to think that the default expect timeout should be applied to all its features until the specific timeout is passed directly to the assertion.

@AdamOakman
Copy link

@mxschmitt I also got confused when I first found out that expect.toPass does not respect default expect timeout set in config - there are many expect.toPass assertions in our project and it would be good to be able to set the default timeout once

@injae-kim
Copy link

there are many expect.toPass assertions in our project and it would be good to be able to set the default timeout once

Hi @mxschmitt! as you can check on above comment by @AdamOakman , I also need some way to set timeout on overall expect.toPass() on my project.

@dev-jonghoonpark made awesome PR #28231 to solve this problem. can you review this PR? thanks! 🙇

@uiii
Copy link

uiii commented Nov 20, 2023

Is there at least a way how to access the default expect timeout from config? I am using toPass inside custom matcher and it is really confusing that this matcher doesn't respect default timeout.

@injae-kim
Copy link

injae-kim commented Nov 21, 2023

  • It's natural to think that the default expect timeout should be applied to toPass until the specific timeout is passed directly to the assertion.
  • I also got confused when I first found out that expect.toPass does not respect default expect timeout,
  • it is really confusing that this matcher doesn't respect default timeout.

For @gskierk, @AdamOakman, @uiii and who that confusing about expect.toPass's default timeout doesn't respect expect's timeout (I also got confused at first but now understand it)

https://playwright.dev/docs/test-assertions#expecttopass

You can retry blocks of code until they are passing successfully.

await expect(async () => {
  const response = await page.request.get('https://api.example.com');
  expect(response.status()).toBe(200);
}).toPass();

As you can see on above toPass doc, toPass basically retry N times until expect are passing successfully.

So If toPass use same default timeout with expect, it can't retry! cause toPass is timeout right after expect is timeout.

  • e.g. (BAD) If expect timeout is 100ms and toPass timeout is 100ms, toPass can't retry
  • e.g. (GOOD) If expect timeout is 100ms and toPass timeout is 500ms, toPass can retry 4 times

https://github.com/microsoft/playwright/blob/main/packages/playwright/src/matchers/matchers.ts#L370

export async function toPass(
  this: ExpectMatcherContext,
  callback: () => any,
  options: {
    intervals?: number[];
    timeout?: number,
  } = {},
) {
  // 👇👇here! toPass default timeout is 0 (it respect test timeout)
  const timeout = options.timeout !== undefined ? options.timeout : 0;

So as you can check on above code, toPass uses default timeout as 0, not uses expect's timeout (it means toPass retry until this test is timeout)

And by PR #28231, you can manually set some default timeout on toPass by config file~!

After checking this, I think maybe it's better to add this description on doc#toPass#timeout 🤔
(it's easily confuse about this, cause there's not enough description about toPass#timeout and how it works with expect's timeout)

@uiii
Copy link

uiii commented Nov 21, 2023

Hi @injae-kim

So If toPass use same default timeout with expect, it can't retry! cause toPass is timeout right after expect is timeout.

This is not fully true, it depends on what type of expect assertions you use. There are two types, auto-retrying and non-retrying.

As you wrote that toPass basically retry N times until expect are passing successfully, it is the same behaviour for all the auto-retrying assertions. Additionally toPass has intervals option to delay following retries.

So if you use auto-retrying assertions in toPass it might be a problem if the timeout is the same, but it is not the case of the example you provided.

The reason why I use toPass is to create custom matcher combining multiple assertions behaving like auto-retrying assertion. It checks if the table has the expected values:

export async function toHaveTableData(locator: Locator, expected: string[][], options?: { timeout?: number }) {
	try {
		await expect(async () => {
			const rows = locator.locator("> tbody > tr");
			const rowCount = await rows.count();

			expect(rowCount, "Table has unexpected number of rows").toBe(expected.length);

			for (let i = 0; i < rowCount; ++i) {
				const rowData = await rows.nth(i).locator("> td").allInnerTexts();
				expect(rowData, `Row number ${i + 1} has unexpected data`).toEqual(expected[i]);
			}
		}).toPass(options);

		return {
			message: () => "Tables data are equal",
			pass: true
		};
	} catch (e) {
		return {
			...e.matcherResult,
			message: () => e.matcherResult.message,
		};
	}
}

So if someone use it is really confusing as it uses different timeout:

const table = page.getByTestId("table");

await expect(table).toBeVisible(); // <--- this uses default expect timeout (5s)
await expect(table).toHaveTableData(tableData /* string[][] */); // <---- this uses test timeout by default (60s)

So I'd like to configure the toPass in my matcher to use the default expect timeout. How, should I get it from playwright.config.ts directly?

Or, is there another way how to combine multiple expects in a custom matcher? Using auto-retrying assertions there, is even more problematic as they should share one timeout (passed to custom matcher), but there is no obvious way how to split it among multiple assertions.

@injae-kim
Copy link

injae-kim commented Nov 21, 2023

@uiii, oh~ thanks for your comment! I didn't know that expect assertions has both auto-retrying and non-retrying type.

export function currentExpectTimeout(options: { timeout?: number }) {
const testInfo = currentTestInfo();
if (options.timeout !== undefined)
return options.timeout;
if (currentExpectConfigureTimeout !== undefined)
return currentExpectConfigureTimeout;
let defaultExpectTimeout = testInfo?._projectInternal?.expect?.timeout;
if (typeof defaultExpectTimeout === 'undefined')
defaultExpectTimeout = 5000;

this.expect = takeFirst(projectConfig.expect, config.expect, {});

expect.toPass({ timeout: currentExpectTimeout() });

So I'd like to configure the toPass in my matcher to use the default expect timeout. How, should I get it from playwright.config.ts directly?

On your case (intentionally want to set expect.toPass's timeout same with expect's timeout), I think you can pass expect's timeout like above code using currentExpectTimeout() in globals.ts!

is this what you want?

@uiii
Copy link

uiii commented Nov 21, 2023

@injae-kim Unfortunately currentExpectTimeout is either not exported or is not included in Typescript typings.

@injae-kim
Copy link

injae-kim commented Nov 21, 2023

@uiii aha 😢 then I think we should use expect timeout directly on playwright.config.ts.

Hmm.. is it worth to make some way to access expect's timeout for user who wants to set expect.toPass() timeout same with expect()? waiting maintainer's answer~

@uiii
Copy link

uiii commented Nov 21, 2023

then I think we should use expect timeout directly on playwright.config.ts.

Yeah, I did this as a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants