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

Better event fetching code for e2e tests #1375

Draft
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

kirill-havryliuk
Copy link
Contributor

Pre-review checklist

  • All code has been formatted using our config (make format)

Mostly, refactoring of subscribeToBrowserEvents function.

-- | race x y = race y x
-- | race x (race y z) = race (race x y) z
-- | race never x = x
race :: forall (a :: Type). Aff a -> Aff a -> Aff a
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this differ from:

race f g = sequential (parallel (try f) <|> parallel (try g)) >>= liftEither

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. This should be the same. Nice catch.

@kirill-havryliuk kirill-havryliuk force-pushed the k.havryliuk/better-event-fetching-code-for-e2e-tests branch from 50e0bbb to d6e2e11 Compare January 3, 2023 16:15
src/Internal/Helpers.purs Outdated Show resolved Hide resolved
let
-- Asyncronously gets browser events and pushes
-- to the `eventVAar` one by one.
watcher = fix \this -> do
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW You don't need to fix under monadic recursion for Aff, ie you can use watcher directly and recursively here (and in the other places you have used fix, if you gave them names)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. That's the matter of taste. I prefer to use fix.

Copy link
Contributor Author

@kirill-havryliuk kirill-havryliuk Jan 3, 2023

Choose a reason for hiding this comment

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

This is just less verbose, imho.

foo bar baz quux = something *> foo bar baz quux
foo bar baz quux = fix \this -> something *> this
foo bar baz quux = let this = foo bar baz quux in something *> this

PS.
Well, foo, has a bunch of arguments, and this is not a honest comparison.
watch = something *> watch
watch = fix \this -> something *> this

For me it is just a matter of taste, as I told earlier. :)

Co-authored-by: Joseph Young <[email protected]>
@kirill-havryliuk kirill-havryliuk force-pushed the k.havryliuk/better-event-fetching-code-for-e2e-tests branch from 8953a81 to cf13dc8 Compare January 3, 2023 17:31
Comment on lines +309 to +310
throwError (error nextAfterConfirmAccess)
]
Copy link
Member

Choose a reason for hiding this comment

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

This effectively sets 100 seconds timeout for every test. It's not what we want, the test suite should control the timeouts, not the testing engine.

Comment on lines +315 to +316
, delaySec (Seconds 100.0) *>
throwError (error nextAfterSign)
Copy link
Member

Choose a reason for hiding this comment

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

Same

Comment on lines +352 to +354
, delaySec (Seconds 10.0) *> throwError
(error nextAfterConfirmAccess)
]
Copy link
Member

Choose a reason for hiding this comment

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

Same

]
Sign -> handler =<< raceMany
[ waitNextEvent
, delaySec (Seconds 10.0) *> throwError (error nextAfterSign)
Copy link
Member

Choose a reason for hiding this comment

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

Same

@klntsky klntsky marked this pull request as draft March 8, 2023 15:43
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.

3 participants