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

Fixed problem with selenium-docker failing #1924

Merged

Conversation

vgoncharenko
Copy link
Contributor

Fixed problem with selenium-docker failing in case for a lot off scenarios in one docker contained without rebutting

This bug caused in case when I tested some urls (10 for example) and in case of failing url 5 for example(random fails sometimes happening in Magento2 =( ), the next steps can't be run with next error:
"Browsertime BrowserError: unknown error: Chrome failed to start: exited abnormallyINFO: Browser failed to start, trying one more time: Failed to start browser in 60 seconds."
I investigated this problems and understand that each run of browsertime starts some process: /usr/bin/dbus-daemon
/usr/lib/at-spi2-core/at-spi-bus-launcher
and don't kill after tested each URL (SUCCESS/FAIL case doesn't matter)

Your checklist for a pull request to sitespeed.io

Please review the guidelines for contributing to this repository.

  • I'm making a big change or adding functionality so I've already opened an issue proposing the change to other contributors, so I got feedback on the idea before took the time to write precious code
  • Check that your change/fix has corresponding unit tests (if applicable)
  • Squash commits so it looks sane
  • Update the documentation https://github.com/sitespeedio/sitespeed.io/tree/master/docs in another PR
  • Verify that the test works by running npm test and test linting by npm run lint

Description

Please describe your pull request and tell us the fix #

Thank you for making sitespeed.io even better!

…off scenarios in one dokcre contained without rebutting
@soulgalore
Copy link
Member

Thank you @vgoncharenko !
I think this should be upstream, seems to be some kind of bug in Chrome/Chromedriver?

I'll test and merge later today and then we can rollout the fix during the weekend (we got some things lined up to be released so that's good). Also I want push this to Browsertime (in master we are working in the new 3.0 so I want sure have it there too).

@tobli have look you too when you have time!
Best
Peter

@vgoncharenko
Copy link
Contributor Author

Yes, in my opinion this bug related to Chromedriver.

Thanks

@soulgalore
Copy link
Member

Cool, if you have the time it would be super helpful if you could raise Chromedriver ticket too.

I did quick try and I get one extra kill that for a process that is already shutdown:

[2018-03-02 09:30:11] INFO: Finished analysing https://www.sitespeed.io/
/start.sh: line 102: kill: (57) - No such process

Do you get the same?

…f scenarios in one docker contained without rebutting

- remove global trap for shutting down background sub-process
@vgoncharenko vgoncharenko force-pushed the fixed-selenium-docker-failings branch from 414291d to 9ff9ca7 Compare March 2, 2018 10:04
@vgoncharenko
Copy link
Contributor Author

Yes, sorry, it happens because I set global trip for background process, but don't forgot to delete after testing, I make this fix some time ago, for my image, and now forgot some details of realisation

@soulgalore
Copy link
Member

Didn't get the time to merge/release this weekend, let me try to do it later this week, sorry.

@soulgalore
Copy link
Member

@vgoncharenko you have this merged in your own test right, or do you need this pushed ASAP? Been working on the coming 3.0 of Browsertime and thinking we should add so we do a hard kill of Chrome/Chromedriver if something fails, so it doesn't leave any stuff behind, that fix would be cleaner.

But let me know, if you need we push it. Else I hope we can release Browsertime sometimes the coming months and then we shortly will release the new sitespeed.io using it.

@vgoncharenko
Copy link
Contributor Author

vgoncharenko commented Mar 10, 2018 via email

@soulgalore
Copy link
Member

Let me merge anyway this since we have others that have the same problem. @vgoncharenko do you have any URL we can reference for your fix (just a comment in the code)? Else I see if I can find one.

@soulgalore soulgalore merged commit 91af79b into sitespeedio:master Mar 20, 2018
@soulgalore
Copy link
Member

thanks @vgoncharenko !

@vgoncharenko
Copy link
Contributor Author

@soulgalore , sorry, my English isn't perfect, can you explain for me part of your sentense about URL? What URL do you need?
And thanks for marge to master. What about 2.x? I can create separate PR if this fix compatible for 2.x.

@soulgalore
Copy link
Member

Ah no problem, I was unclear. I meant did you come up with your fix yourself or do you have any reference?

I can add it to Browsertime later on, lets see first when we have a plan for releasing 3.0.

Thanks again!

@vgoncharenko
Copy link
Contributor Author

@soulgalore , yes, it's my fix

Ok, thanks
Waiting new image)

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