-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Test on IE11 and Safari #3636
Comments
@devoto13 I added you to the browserstack and saucelabs teams. Let me know if there is anything else I can do. |
@johnjbarton Thanks! I've joined the teams and will get back once I have any results. |
Fixed running in IE 11 with #3642. Or at least it works when I run tests in my fork. Let's see if Travis gives any issues after the merge. Safari is running tests, but it throws "Some of your tests did a full page reload!" after the test run is successfully completed. Debugging it. |
Safari had a race condition with the new logic for detecting undesired navigations. It should be fixed with #3643. There is one more tiny fix to the test code necessary to enable BrowserStack tests in Safari, I have it in a branch, will rebase and submit once the above two PRs are merged as it will not apply cleanly on its own.
It would be very helpful to enable browser tests on the PRs, but I can't think of a way how to do it without exposing BrowserStack credentials publicly (thus allowing anybody to use them for testing anything they wish). Others seem to do so... Maybe we can do the same and deal with the consequences if anybody attempts to abuse it 🤔 |
@johnjbarton What do you think about the last point in my previous comment? |
We actually face the same issue upstream on Bootstrap. In the past we had a bot that run the tests for us, but we dropped it. So, yeah, we don't even test PRs for years now. |
@XhmikosR Can you share a bit more about your reasoning for dropping the bot? Too hard to maintain? Don't feel a need?
What is your experience? Do you often face post-commit failures? How do you deal with them? Do you rely on other tools (like ESLint you've pointed out earlier) to catch most of the browser compatibility problems? |
I definitely see the need but I haven't found an easy way of implementing this. twbs/bootstrap#27551. So yeah, lack of knowledge and manpower to maintain such a tool :/ I'd like to have this though.
Everything else runs on CI apart from BrowserStack. There are a few times we might get something broken to land, but we fix it. I also experimented with eslint-plugin-compat but it doesn't catch everything and seems to have some issues. So, for now, since we use Babel (but not its polyfills), we basically rely on BrowserStack for real browser compatibility. |
I will look into ensuring the tests run in precommit. |
This was mentioned earlier but the fundamental problem is that we either share our browserstack credentials with the internet or don't run on pre-commit. I looked but I could not find similar info for Github actions or CircleCI (used by angular team). But I don't know if this is because these CI systems have a solution or just don't document their failure the way Travis does. I'm trying to find this out. Contributors with write privilege can use the For the time being let's just try to catch failures on post-commit promptly. Sadly it also appears that we have to look for the little red X in the right place or read the travis emails carefully to sort out when the fail happens. |
Closing since the basic work by @devoto13 solves the testing. |
We removed the IE11 and Safari tests to release v6. I could not get BrowserStack to work.
The problem may have occurred long ago as it turns out these tests were only running on post-commit. That much is cleaned up.
The browserstack tests launch the remote browser and push the karma .html file URL, but karma client never gets back to the server. Chrome and Firefox work, so I suspect that browserstack may run these native on linux while the other two are running in other environments/virtual machines.
The text was updated successfully, but these errors were encountered: