-
Notifications
You must be signed in to change notification settings - Fork 77
Increase jest timeout for PWDEBUG option #596
Changes from all commits
978aecd
1f582c1
6d44157
981b70d
b5a29f5
932bad2
f543e19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ import { | |
generateKey, | ||
} from './utils' | ||
import { | ||
DEBUG_TIMEOUT, | ||
DEFAULT_TEST_PLAYWRIGHT_TIMEOUT, | ||
CONFIG_ENVIRONMENT_NAME, | ||
SERVER, | ||
|
@@ -95,15 +96,22 @@ const getDevices = ( | |
return resultDevices | ||
} | ||
|
||
const getJestTimeout = (configTimeout?: number) => { | ||
if (configTimeout) { | ||
return configTimeout | ||
} | ||
return process.env.PWDEBUG ? DEBUG_TIMEOUT : DEFAULT_TEST_PLAYWRIGHT_TIMEOUT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mmarkelov I think you should document that you set the default timeout to 15s. I was surprised when running tests after a recent upgrade that it increased from 5 -> 15 s for each jest test. It is worth mentioning in your docs that this is the case. Also, https://playwright.dev/docs/next/debug/#run-in-debug-mode mentions that it both runs in headful, as well as sets the playwright timeout to 0 (= no timeout). Is this the same you do? I.e. set both Jest's and Playwrights timeout to no timeout? I think it is worth mentioning in the docs what you do when a user sets There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jhildenbiddle i believe that changes from 5 -> 15 s was made in the first versions of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mmarkelov Did you mean to tag me really? :) Could you document this? Two things I think of.
I think that would be great :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thernstig create new issue about doc improvements #602 |
||
} | ||
|
||
class PlaywrightRunner extends JestRunner { | ||
browser2Server: Partial<Record<string, BrowserServer>> | ||
constructor( | ||
globalConfig: JestConfig.GlobalConfig, | ||
context: TestRunnerContext, | ||
) { | ||
const config = { ...globalConfig } | ||
// Set default timeout to 15s | ||
config.testTimeout = config.testTimeout || DEFAULT_TEST_PLAYWRIGHT_TIMEOUT | ||
// Set testTimeout | ||
config.testTimeout = getJestTimeout(config.testTimeout) | ||
super(config, context) | ||
this.browser2Server = {} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does
jestPlaywright.debug()
interact withpage.pause()
? https://playwright.dev/docs/api/class-page?_highlight=paus#pagepauseIt would be great if
jestPlaywright.debug()
also pauses (and allows the user to resume) but I think this should be explicitly explained here. It is good to say here what debugging does both for the Playwright settings, as well as the Jest settings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can rework that jestPlaywright will use playwright.pause under the hood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhildenbiddle
page.pause()
was introduced only in last version. AndjestPlaywright.debug()
does not related to this function. Personally, I don't likejestPlaywright.debug()
because it's weird and hacky solution to simplify debugging, that came fromjest-puppeteer
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mxschmitt yes we can. But it will be breaking change. I think usage of this feature for debugging is much better that
jestPlaywright.debug()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be sweet, do you want to create an issue? I can do it if you want but you can probably better explain it. Release it under a 2.0 release then, I think this awesome project has graduated enough to warrant a new major version 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thernstig yes. It will be nice if you create an issue)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#608