Fix ngjq #11182
Fix ngjq #11182
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
|
Looks good so far. We ought to be able to work out what jquery-like library is being used from inside the fixture app. For instance, in the fixture app, call |
|
We can just check that |
|
thx @gkalpak that works very well. I'm not quite sure how we should test for another version of jquery without including it in the repo. |
|
Since it is just an e2e test, perhaps we could hit a CDN with well defined versions of jQuery for the test? |
|
I updated the PR with expects & a test for ensuring forcing a specific jQuery version works via google cdn. The http request should be negligible as the e2e take a long time anyway. |
| it('should be able to force jqlite', function() { | ||
| loadFixture("ngJq").andWaitForAngular(); | ||
| expect(browser.executeScript('return window.angular.element !== window.jQuery')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
It might be a good idea to also test that window.jQuery is defined.
E.g. imagine 'jquery.js' isn't properly loaded in the future for whatever reason (e.g. directory is moved). It would be good to catch this in the test (and fix).
|
I refactored the tests to use a directive that exposes a binding on the scope. The test with forcing a specific version of jQuery fails when the file is on the google cdn. It works when I have the file locally. It also works with the cdn when I try it in a non-protractor environment, so I think it might be something with protractor, that it is not waiting for the file to download. |
|
I think the e2e middleware is rewriting the path to the CDN for the jquery at this point: https://github.com/angular/angular.js/blob/master/test/e2e/tools/util.js#L34 |
|
In my branch here: angular:master...petebacondarwin:ngJq then run and go to http://localhost:8000/e2e/fixtures/ngJqJquery You can see in the source that the CDN src is changed to |
|
Thanks @petebacondarwin I was so focused on protractor being the problem that I didn't consider the rewrite. |
|
I updated the PR. script src starting with |
|
And now it works, yes? |
| @@ -0,0 +1,6 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
This line breaks jshint rules. It should either be made into the functional form, or perhaps easier just remove it.
|
Merging... |
|
Thanks @petebacondarwin ! |
@mboudreau This is a wip for the e2e tests. You can play around with it. (use
grunt test:protractor) You can als comment out the other test paths inprotractor.conf.js, so only the e2e folders get run:The problem is currently that we don't have a reliable method of detecting what is loaded, because
jqis private. We might have to expose it. Currently we can only test that it doesn't break.