Should we remove the 10s sleep after initializing laravel ?

chilio / laravel-dusk-ci

Docker Test suite for Laravel Dusk in gitlab CI

MIT License

159 stars 51 forks source link

Should we remove the 10s sleep after initializing laravel ? #42

Closed ice-blaze closed 5 years ago

ice-blaze commented 5 years ago

On this line https://github.com/chilio/laravel-dusk-ci/blob/1dd4abbeafb1334e28f44b3b2227d029fdda4c99/commands/configure-laravel.sh#L14 I would see an environment variable instead of an hard code 10s.

chilio commented 5 years ago

We could move this to ENV, but it is needed to ensure system chromedriver is started before the one shipped with laravel.

ice-blaze commented 5 years ago

I see two solutions here:

Having an environment variable, and if not defined, use the 10 seconds as default.
Wait until the chrome driver is really started. With a while loop waiting on the driver.

Which one do you prefer? (I would say the cleanness is the second one)

chilio commented 5 years ago

For sure second one is much more cleaner...

chilio commented 5 years ago

@ice-blaze second option is now embedded in dev. Can you verify, if it is working, the way you expected?

ice-blaze commented 5 years ago

Hi, wow thanks I thought I had to put my head into this feature ^^'

Well I still have the sleep 10s https://github.com/chilio/laravel-dusk-ci/blob/dev/commands/configure-laravel.sh#L14 in the configure-laravel

I tried without the sleeep 10s and it worked but it also work on the stable release so I guess my PC is too fast.

chilio commented 5 years ago

Hi, sure I forgot to remove the sleep.... And you are true, if it works without sleep on your machines, they are fast enough. This was intended to doublecheck for really slow CI environments, and your suggested solution saves few seconds...

ice-blaze commented 5 years ago

@chilio Sure, thank you for the modifications! Issue closed :)