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

closes #95 #162

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

rohankatyal29
Copy link
Contributor

#95

@rohankatyal29
Copy link
Contributor Author

@ankit-m - what is this check that is failing?

@ankit-m
Copy link
Contributor

ankit-m commented Jul 22, 2016

@rohankatyal29 can you check it on your local machine?

@ankit-m
Copy link
Contributor

ankit-m commented Jul 22, 2016

The TravisCI log shows that PhantomJS did not load. My guess is that mutation observer is causing it to fail. Is Mutation Observer supported in Chrome only? The test environment uses PhantomJS as it is headless.

@rohankatyal29
Copy link
Contributor Author

screen shot 2016-07-22 at 12 49 27 pm

Can we just ignore this?

@ankit-m
Copy link
Contributor

ankit-m commented Jul 22, 2016

@rohankatyal29 I don't think this will be a good idea. Because if we merge this, all Travis CI checks will fail from now on, defeating its purpose. Would it be possible to find a workaround?

Karma does support Chrome and Chrome Canary but with the downside that every test will open a window of Chrome. But I don't know if it will work on Travis (as it is only terminal).

As an alternative, maybe we can delay this change until headless Chrome is released. It is supposed to be out soon. https://news.ycombinator.com/item?id=11839303

@rohankatyal29
Copy link
Contributor Author

@ankit-m, @ceilican - Maybe we can merge this is in the production version when we release it to the users, but not put in the dev repo. What do you think?

@ceilican
Copy link
Contributor

@rohankatyal29 I don't think this will be a good idea. Because if we merge this, all Travis CI checks will fail from now on, defeating its purpose. Would it be possible to find a workaround?

I think any workaround would have to be found in the way we do the tests. We shouldn't hack our solutions just because they are not testable with current testing technology. Testing is very important, but it is secondary to the main goal.

@ankit-m, @ceilican - Maybe we can merge this is in the production version when we release it to the users, but not put in the dev repo. What do you think?

That would be inconvenient to maintain.

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.

3 participants