Open rafael-leal-mccormack opened 9 months ago
Thanks! This makes sense to me, but I'd like to confer with the rest of the team first before we move forward with this. I'll be in touch!
Whoops - sorry, started to update the wrong issue 😓
After discussing with the team, I think we'd like to move forward with this!
If you're interested in putting up a PR, I think you'd need to do the following:
stencil-public-compiler.ts
's definition of the TestingConfig
interface, which is where test settings are declared in a user's stencil.config.ts
, to add an optional screenshotTimeout
field - roughly here, after 'screenshotConnector'. This value should probably accept a value of null
here as well, as I can imagine we'll want that in our next step. When you're done, this should probably look something like:
/**
* Path to the Screenshot Connector module.
*/
screenshotConnector?: string;
2. Next, the compiler will need to verify a user provided value from `stencil.config.ts`. While we said `screenshotTimeout` to be an optional field that could be a `number | null`, that doesn't stop folks from putting _any_ value in their configuration file. So, we'll want to add a simple check to our `validate-testing.ts` file, which does these types of checks. The best place to add a validation is probably [here, right after our screenshotConnector validation](https://github.com/ionic-team/stencil/blob/bb0b1d46f63175dc09d0a23445be4d4a0d891a01/src/compiler/config/validate-testing.ts#L73). For this, it's probably best that we just set the value of `testing.screenshotTimeout` to `null` if `typeof screenshotTimeout` isn't a number:
```ts
if (typeof testing.screenshotTimeout != 'number') {
testing.screenshotTimeout = null;
}
That way, when we go to use this value later, we can make the distinction between a value of '0' and null
later on.
Next, we should probably unit test our code we just wrote in the previous step. There's a test suite for the file we just edited here, which tests each field in TestingConfig
. Like we did previously, we can put these tests just after the screenshotConnector
tests, here. There are three cases we should probably test regarding the value of screenshotTimeout
- one where the value is undefined
, one where it's null
, and one where it's a number
(maybe even a couple number
use cases, so we test '0' and a non-falsey value!). In general, a single test will probably look something like:
it(`assigns the screenshotTimeout value to 'null' when 'null' is provided`, () => {
userConfig.flags = { ...flags, e2e: true }; // we need this line of setup to tell the validator we're gonna do e2e config validation
userConfig.testing = { screenshotTimeout: null }; // this line changes to provide `screenshotTimeout` to the tests
const { config } = validateConfig(userConfig, mockLoadConfigInit()); // ACT stage, validate our testing config
expect(config.testing.screenshotTimeout).toBe(null); // assert
});
Next, we need to pass this configuration data to the testing module of Stencil. This is done today through environment variables. To set this up, we need to:
Add a field to our E2EProcessEnv
for the screenshot timeout
__STENCIL_SCREENSHOT_BUILD__?: string;
+ __STENCIL_SCREENSHOT_TIMEOUT_MS__?: string;
Set __STENCIL_SCREENSHOT_TIMEOUT_MS__
in the Stencil Testing module. It's probably best we do this when the compiler knows it's doing screenshot testing, which is right here:
if (doScreenshots) {
env.__STENCIL_SCREENSHOT__ = 'true';
+ if (config.testing.screenshotTimeout != null) {
+ env.__STENCIL_SCREENSHOT_TIMEOUT_MS__ = config.testing.screenshotTimeout;
+ }
Next, we pull that stringified number out during testing time:
if (typeof env.__STENCIL_SCREENSHOT_BUILD__ !== 'string') {
throw new Error(`compareScreenshot, missing screen build env var`);
}
Now, let's hop on over to getMismatchedPixels
, where we want to use this value:
- const timeout =
+ const timeout = screenshotTimeoutMs !== null ? screenshotTimeoutMs :
typeof jasmine !== 'undefined' && jasmine.DEFAULT_TIMEOUT_INTERVAL
? jasmine.DEFAULT_TIMEOUT_INTERVAL * 0.5
: 2500;
const tmr = setTimeout(() => {
reject(`getMismatchedPixels timeout: ${timeout}ms`);
}, timeout);
This will allow us to use the value if it's set and have it take precedence over other values.
Finally, we just need to pass timeoutMs
down into getMismatchedPixels
:
- async function getMismatchedPixels(pixelmatchModulePath: string, pixelMatchInput: d.PixelMatchInput) {
+ async function getMismatchedPixels(pixelmatchModulePath: string, pixelMatchInput: d.PixelMatchInput, screenshotTimeoutMs: number | null) {
Update the callsite to getMismatchedPixels
in compareScreenShot
:
screenshot.diff.mismatchedPixels = await getMismatchedPixels(
screenshotBuildData.pixelmatchModulePath,
pixelMatchInput,
+ screenshotTimeoutMs,
);
Ensure that compareScreenshot
has screenshotTimeoutMs
:
export async function compareScreenshot(
emulateConfig: d.EmulateConfig,
screenshotBuildData: d.ScreenshotBuildData,
currentScreenshotBuf: Buffer,
desc: string,
width: number,
height: number,
testPath: string,
pixelmatchThreshold: number,
+ screenshotTimeoutMs: number | null,
) {
And finally, update the callsite for compareScreenshot
:
const results = await compareScreenshot(
emulateConfig,
screenshotBuildData,
screenshotBuf,
desc,
width,
height,
testPath,
pixelmatchThreshold,
+ screenshotTimeoutMs,
);
From there, that should be all the changes you need to make!
To test this, you can build the project with npm ci && npm run build && npm pack
- this'll create a tarball file in your local fork of the Stencil repo. From there, you can install it in any project like a normal NPM dependency (e.g. npm i PATH_TO_GENERATED_TARBALL
) and test these changes out.
Let me know if you have any questions!
Prerequisites
Describe the Feature Request
Can we add a stencil config option here https://github.com/ionic-team/stencil/blob/54d4ee252768e1d225baababee0093fdb0562b83/src/screenshot/screenshot-compare.ts#L140-L143 to increase the timeout from jasmine? Currently this isnt changeable from 2500 for anyone on v28+ of jest because jest no longer uses jasmine as the runner and uses jest-circus.
Describe the Use Case
The use case is for users with a stencil repo who are using jest v28+ and also including long running screenshot tests in their project.
Describe Preferred Solution
A property in stencil.config.ts maybe called
screenshotTimeout
that will be used ifjasmine
doesnt exist in the above mention file.Describe Alternatives
An env variable that could be set instead of a property in the stencil config could be a solution. Stencil config would be more direct though and less chances of failure if CI's or local computers have random variables set.
Related Code
No response
Additional Information
No response