chore: remove inactive browserstack wrapper package #1991
Conversation
googlebot
added
the
cla: yes
devversion
changed the title
devversion
force-pushed
the
chore/use-own-wrapper
branch
from
e17b70c to
d44c4ae
Compare
No longer uses the "inactive" browserstack wrapper package from npm, which was just responsible for creating the readyfile. We can create the readyfile ourself and get rid of the npm package.
devversion
force-pushed
the
chore/use-own-wrapper
branch
from
d44c4ae to
6b5662b
Compare
|
|
||
| cd $TUNNEL_DIR | ||
|
|
||
| # Download the saucelabs connect binaries. |
There was a problem hiding this comment.
"saucelabs" (here and other places in this file)
There was a problem hiding this comment.
Good catch. I made the two files in the material.angular.io CI setup as similar as possible (so we can share a lot)
|
@hansl can you take a look at this? |
|
|
||
| echo "BrowserStack Tunnel ready" | ||
|
|
||
| touch $BROWSER_PROVIDER_READY_FILE |
There was a problem hiding this comment.
Why use killall instead of output the PID of browserstack in the ready file?
There was a problem hiding this comment.
I changed it to killall because we are doing the same with sauce-connect and it would be consistent with the readyfile API from sauce-connect
There was a problem hiding this comment.
I'm not 100% convinced yet that this is right; what if you run these scripts locally on your computer?
There was a problem hiding this comment.
I can see what you mean, but I think you won't be able to fix this for saucelabs, because the integrated --readyfile option can't write any PID to the file.
|
|
||
| # Disown the background process, because we don't want to show any messages when killing | ||
| # the timer. | ||
| disown |
There was a problem hiding this comment.
I don't think you need to disown here, since you're &> /dev/null below. Right?
There was a problem hiding this comment.
The &> /dev/null won't have an effect on that, because when using the kill command the process always prints a message that the child-process terminated.
|
|
||
| # When the tail recognizes the `Ctrl-C` log message the BrowserStack Tunnel is up. | ||
| { | ||
| tail -n0 -f $TUNNEL_LOG --pid $TIMER_PID | { sed '/Ctrl/q' && kill -9 $TIMER_PID; }; |
There was a problem hiding this comment.
If it takes more than 2 minutes the tail will exit. We should treat that case since it's an error and it would try to connect later on to browser stack but would fail (since the tunnel isn't connected but this script succeeded).
The way to do that would be (my BASH is rusty) to, instead of just sleep 120 & do something like
(sleep 120; echo ERROR TIMEOUT > $BROWSER_STACK_ERROR_FILE) &then on the wait tunnel script, if ERROR_FILE exists just exit with a special code that fails the test.
What do you think?
There was a problem hiding this comment.
The tail should only run 2 minutes at maximum. If the specific string was found then the $TIMER_PID will be killed and the tail exits.
Did you mean something else?
There was a problem hiding this comment.
Yes, if the tail runs for longer than 2 minutes because the connection to the tunnel takes longer. The timer will terminate but the process will succeed.
There was a problem hiding this comment.
Got it. Yeah I like that idea with the error file. Will change that
|
Just fix the timeout issue and you'll get my LGTM. Good work! |
|
@hansl Done :) |
| @@ -12,6 +13,11 @@ while [ ! -f $BROWSER_PROVIDER_READY_FILE ]; do | |||
| echo | |||
| echo "Timed out after 2 minutes waiting for tunnel ready file" | |||
| exit 5 | |||
| elif [ -f $BROWSER_PROVIDER_ERROR_FILE ]; then | |||
There was a problem hiding this comment.
There's a race condition here, since READY_FILE will still be created when ERROR_FILE exists. Moving this IF outside the while will ensure we always take care of this case.
|
1 small nit and LGTM! Thanks |
|
@hansl Good point -Fixed. Thanks again for your great comments! |
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No longer uses the "inactive" browserstack wrapper package from npm, which was just responsible for creating the readyfile.
We can create the readyfile ourself and get rid of the npm package.