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

🐛 Account for logger websocket synchrony #284

Merged
merged 1 commit into from
Apr 3, 2021

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented Apr 2, 2021

What is this?

Sometimes, WebSocket errors can come from the constructor itself which is defined outside the scope of the logger. Rather than having extra error handling, we can have the logger accept a socket creator function and wrap it with the existing error handling.

Usage of the remote method was updated in @percy/sdk-utils as well as other tests.

@wwilsman wwilsman added the 🐛 bug Something isn't working label Apr 2, 2021
@wwilsman wwilsman requested a review from Robdel12 April 2, 2021 22:01
@wwilsman wwilsman enabled auto-merge (squash) April 2, 2021 22:03
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁

@wwilsman wwilsman force-pushed the ww/logger-socket-synchrony branch 6 times, most recently from f432d6e to 3ea0014 Compare April 3, 2021 04:57
@wwilsman wwilsman force-pushed the ww/logger-socket-synchrony branch from 3ea0014 to 3cc4083 Compare April 3, 2021 05:00
@wwilsman wwilsman merged commit c2a6d33 into master Apr 3, 2021
@wwilsman wwilsman deleted the ww/logger-socket-synchrony branch April 3, 2021 05:05
@wwilsman
Copy link
Contributor Author

wwilsman commented Apr 3, 2021

Weird. Looks like automerge kicked in even though there were failing checks

@Robdel12
Copy link
Contributor

Robdel12 commented Apr 3, 2021

Not marked required are they?

@wwilsman
Copy link
Contributor Author

wwilsman commented Apr 3, 2021

No they are required. It must've been a race condition where the checks showed as passing before previously passing checks reran and failed

@wwilsman
Copy link
Contributor Author

wwilsman commented Apr 3, 2021

Broken test was a silly windows issue fixed with 3c94d7e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants