Skip to content

Fix race conditions(?)#166

Merged
jwoertink merged 5 commits into
luckyframework:mainfrom
wout:fix-race-conditions
Apr 18, 2026
Merged

Fix race conditions(?)#166
jwoertink merged 5 commits into
luckyframework:mainfrom
wout:fix-race-conditions

Conversation

@wout
Copy link
Copy Markdown
Contributor

@wout wout commented Apr 6, 2026

I'm mildly optimistic about this update. I haven't seen any errors so far.

SESSION_RETRY_LIMIT

The biggest issue I think was the @retry_limit instance variable in the selenium driver class. Any delays in the setup, after initialization and before the actual tests were run, would eat into the time span. Now the full time span is considered.

The wait_for_ready method

Waits for the DOM to be ready. That's just a precaution,

Navigate to about:blank on reset

Just to make sure no remnants of the previous test are left. Essentially always starting with a blank slate.

LuckyFlow.wait_for_server

This is a small addition to apps to make sure the app's http server is ready to receive requests. So the spec/setup/start_app_server.cr file now looks like:

app_server = AppServer.new

spawn do
  app_server.listen
end

LuckyFlow.wait_for_server

Spec.after_suite do
  LuckyFlow.shutdown
  app_server.close
end

wout added 4 commits April 6, 2026 13:41
In most cases this will be negligible, but it's another safety.
Becuase the `@retry_limit` time span was initialised with the object,
the time had already partially elapsed becaus of any setups that would
happen afterwards. This fix makes sure the full time span is available
from the moment the session is started.
This is just a precaution because in most cases the DOM should be ready
Cookies were already cleared, but any remants form the previous DOM
could still be present. This makes sure the DOM starts fresh every time.
@wout wout changed the title Fix race conditions Fix race conditions(?) Apr 6, 2026
@jwoertink
Copy link
Copy Markdown
Member

😱 omg, will this fix the issues I was having that I mentioned in luckyframework/lucky_cli#890 ? That would be amazing 🙌

@wout
Copy link
Copy Markdown
Contributor Author

wout commented Apr 6, 2026

I don't know, that's why I added the question mark to the title 😄. This could fix it, but you have to see for yourself. So pull in my branch before merging it in. At least this makes sure that everything is ready and set up before actually testing, and that there's nothing left in the DOM from previous tests, which could also be a source for weird errors.

@jwoertink
Copy link
Copy Markdown
Member

May potentially (hopefully) fix luckyframework/lucky_cli#883 .. 🤞

@jwoertink
Copy link
Copy Markdown
Member

I can't tell if this made it better or not... but there was still a CI failure

https://github.com/luckyframework/lucky_cli/actions/runs/24057314088/job/71300227677?pr=925#step:10:133

However, there was only 1 failure, so it's possible this made a difference. I'm just not sure how to tell 🤔

@wout
Copy link
Copy Markdown
Contributor Author

wout commented Apr 18, 2026

Hmm, I haven't seen any so far but this looks like a timing issue again. I'll have a look at it today.

Are you seeing less errors than before? Or about the same?

@wout
Copy link
Copy Markdown
Contributor Author

wout commented Apr 18, 2026

Another tiny fix based on the error you linked, particularly this line:

https://github.com/luckyframework/lucky_cli/actions/runs/24057314088/job/71300227677?pr=925#step:10:140

It may be that the references from the driver.find_css(selector) call got stale by the time element.text is called.

This is a bit like shooting in the dark 😄, but if we keep tracing them down I suppose we'll get there eventually. I know it's not the prettiest fix, but at least it rules out the timing issue here.

@jwoertink
Copy link
Copy Markdown
Member

oh snap! luckyframework/lucky_cli#925 It's green! I think this may have worked :D

Copy link
Copy Markdown
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

I say we give it a shot. Can't be worse than what exists now where Flow specs don't even run 😂 The fact that I was able to take existing specs and just run them and get all green is very promising 🙌

@jwoertink jwoertink merged commit 7892033 into luckyframework:main Apr 18, 2026
7 checks passed
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.

2 participants