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

Fix tests after #44 is merged #48

Closed
wants to merge 1 commit into from

Conversation

sagikazarmark
Copy link
Collaborator

@sagikazarmark sagikazarmark commented Oct 8, 2022

Fixes test failures introduced in #44

Signed-off-by: Mark Sagi-Kazar mark.sagikazar@gmail.com

Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
@sagikazarmark sagikazarmark changed the title test: fix race in test introduced in #44 Fix tests after #44 is merged Oct 8, 2022
@sagikazarmark sagikazarmark force-pushed the fix-race-introduced-in-44 branch 2 times, most recently from a0433b4 to d97c498 Compare October 8, 2022 08:39
@sagikazarmark
Copy link
Collaborator Author

@gagern can you please take a look at the breaking tests? I have no idea why CI wasn't triggered for #44, but it introduced a number of breaks. I fixed the first one, but the second one is not that trivial (the test passes locally, fails on CI).

@sagikazarmark
Copy link
Collaborator Author

@gagern any chance you could look into this?

There is also #49 that was reported for master.

I really-really don't want to, but I'll be forced to revert that awesome change if we can't figure these issues out. :/

@DPJacques
Copy link
Collaborator

I will take a look at this over the rest of the holiday break and see if I can come up with something.

I need to familiarize myself with what #44 added. I know it included AfterFunc and uses callbacks rather than channels, but I am unfamiliar with the particulars, specifically the "Otherwise it addresses a number of bugs, mainly around out-of-order or duplicate delivery of events, as exposed by the various tests added by these changes."

@DPJacques
Copy link
Collaborator

Figured it out. Fixes incoming. Pardon me as I am still figuring out Github.

@sagikazarmark
Copy link
Collaborator Author

Appreciate it!

@DPJacques
Copy link
Collaborator

Turns out this was mostly a timeout issue. I made #51 to address that and standardize a bit.

While I was going down the rabbit hole, I the control of faceClock.l challenging to follow. One can puzzle it out and eventually understand it, but I wanted it to be more evident to an uninformed reader. My goal was to make it easier to spot bugs and gotchas while hopefully making future contributions easier. Those changes are in #52 . Please let me know what you think.

#51 and #52 are no-overlapping changes.

@sagikazarmark
Copy link
Collaborator Author

This is done

@sagikazarmark sagikazarmark deleted the fix-race-introduced-in-44 branch January 18, 2023 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants