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

Wait for Percy to idle before screenshot task finishes #3767

Merged
merged 3 commits into from
Jun 9, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jun 7, 2023

This PR is a workaround for a potential Percy CLI bug affecting:

percy exec -- npm run test:screenshots

To avoid a potential race condition (see discussion) I've included:

  1. Wait for Percy using waitForPercyIdle() from @percy/sdk-utils
  2. Run Percy asset discovery concurrently (again)
  3. Take Percy snapshots sequentially (again)
import { waitForPercyIdle } from '@percy/sdk-utils'

// Take snapshots
// … then wait for Percy

await waitForPercyIdle()

Console output with percy exec --verbose

Percy continues to show lots of unfinished activity for resource and snapshot uploads after the await percySnapshot() promises have resolved and npm run test:screenshots outputs Finished 'screenshots'

I've attached the full log output percy-exec.log or see the following excerpt:

[percy:core:page] Page closed (2ms)
[percy:client] Finalizing snapshot 1558916655... (37ms)
[17:17:45] Finished 'screenshots' after 17 s
[percy:client] Creating snapshot: js: error-message... (283ms)
[percy:client] Uploading resources for 28033806... (362ms)
[percy:client] Uploading resource: /percy.1686154655270.log... (1ms)
[percy:client] Finalizing snapshot 1558916705... (412ms)
[percy:client] Creating snapshot: js: error-summary... (242ms)
[percy:client] Uploading resources for 28033806... (318ms)
[percy:client] Uploading resource: /percy.1686154655567.log... (0ms)
[percy:client] Finalizing snapshot 1558916738... (413ms)
[percy:client] Creating snapshot: no-js: details... (315ms)
[percy:client] Uploading resources for 28033806... (412ms)
[percy:client] Uploading resource: /percy.1686154655870.log... (0ms)

@colinrotherham colinrotherham added 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) assurance labels Jun 7, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3767 June 7, 2023 16:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3767 June 8, 2023 19:31 Inactive
This attempts to solve a race condition where `percy exec` starts “finalizing” the Percy build early

Even after the `percySnapshot()` promise resolves, resource and snapshot uploads are still in progress

See: percy/cli#1137 (comment)

This commit also uses a tighter `peerDependencies` range to bump the minimum Percy SDK utils version from 1.24.0 to 1.25.0, maybe this will help too
This restores the default 5x concurrent browsers for asset discovery after an attempted fix in: 75d047d
Percy runs healthchecks in each `percySnapshot()` call and might cause a potential race condition
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3767 June 9, 2023 09:43 Inactive
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for digging into that once more! 🙌🏻 ⛵

@colinrotherham
Copy link
Contributor Author

Cheers for digging into that once more! 🙌🏻 ⛵

Thanks @romaricpascal

Percy issues seem to have settled down again now

Maybe their backend services were slow to finish uploads, but hopefully waitForPercyIdle() helps next time

@colinrotherham colinrotherham merged commit 2e7f92d into main Jun 9, 2023
21 checks passed
@colinrotherham colinrotherham deleted the percy-idle branch June 9, 2023 10:59
colinrotherham added a commit that referenced this pull request Dec 11, 2023
We first added the workaround in:
#3767

The Percy issue was fixed in:
percy/cli#1137 (comment)
colinrotherham added a commit that referenced this pull request Dec 11, 2023
We first added the workaround in:
#3767

The Percy issue was fixed in:
percy/cli#1137 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance 🐛 bug Something isn't working the way it should (including incorrect wording in documentation)
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants