-
Notifications
You must be signed in to change notification settings - Fork 521
test: increase the timeout for the image generation test which failed on CI #5573
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
base: develop
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This pull request addresses a flaky test that consistently times out on macOS CI systems by conditionally skipping it only on macOS platforms.
Changes:
- Added
isMac()helper function to detect macOS using user agent - Modified test to skip on macOS while continuing to run on other platforms
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| client/test/helper/index.js | Added isMac() utility function to detect macOS platform via user agent |
| client/src/app/util/tests/generateImageSpec.js | Modified "should automatically handle large images" test to conditionally skip on macOS using the new helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| it('should automatically handle large images <' + type + '>', async function() { | ||
| (isMac() ? it.skip : it)('should automatically handle large images <' + type + '>', async function() { |
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.
Does it fail consistently? I'd rather not blindly disable this - we'll otherwise risk not being able to generate large images on MacOS 😉
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.
I've noticed it yesterday at [deps: update to [email protected] #5572] (https://github.com/camunda/camunda-modeler/pull/5572) PR and also found the same issue at another new PRs, e.g. - #5553. Tried to re-run the job a second time and still got the same error. It seems like something happened with GitHub Actions on macOS, perhaps they've become slower.
As an alternative, we can increase the timeout for this test specifically for macOS. However, it was already increased to 10,000 ms, and further increases could result in longer test runs. Or, I can dive into the test implementation to see if we can optimize it and make it faster, though I'm not sure if that's possible.
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.
Would be good if you could have a quick look, i.e. does it fail in the browser for you?
npm run client:auto-test
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.
No, it fails only at CI
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.
With the new commit at #5572 the error goes out. So it seams to be a flaky.
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.
Would be good if you could have a quick look
The test itself looks pretty straightforward. I think the problem is the size of the canvas we are rendering, as a canvas of that size is too slow to process. We use an SVG with an initial size of 3833 x 8573 pixels and then scale it by 3x, 2x, and 1x. So, in the worst case, we have an image size of 11,499 x 25,719 pixels.
Does an image that large make sense and is it testing a real-world use case? Perhaps we can change the initial size to something smaller, like 1500 x 2000?
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.
We have to ensure it works with our users diagrams, i.e. this.
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.
We have to ensure it works with our users diagrams, i.e. this.
In this case, we also have a pretty large initial SVG (5132 x 1832 pixels).
If we want to keep the tests for such large images, the only way I see to fix the flaky behavior is to increase the timeout once more, e.g., to 15s. We can do it only for macOS env.
I don't see a problem with the test itself, but it performs heavy operations that seem to take a lot of time on the macOS CI runners. I'm not sure if we can optimize it.
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.
its a known issue that the github macos runners have a lot of performance issues (especially storage, not sure if we are writing the image). In another project we setup our own mac mini as a runner as the github one where just too slow. (eg a bit old actions/runner-images#1336)
maybe we can setup the timeout really high and measure the time it actually takes on the runner to see how it fluctuates
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.
f153aac to
b31b6bd
Compare
b31b6bd to
9c10e9e
Compare
… on CI with the previous timeout
9c10e9e to
97701c5
Compare
|
Let's check if the test pass wasn't just a fluke. |
After a few more runs it still fails even with |
|
So a simple timeout increase is not a solution. |
Proposed Changes
test: increase the timeout for the image generation test which failed on CI with the previous(10000ms) timeout
Checklist
Ensure you provide everything we need to review your contribution:
Closes {LINK_TO_ISSUE}orRelated to {LINK_TO_ISSUE}@bpmn-io/srtool