Update tests for AngularJS 1.2 #1342
Conversation
|
@chrisirhc nice work, AngularJS 1.2 support is next on out TODO list after Bootstrap3 support is released. As far as hacks go - those need to be addressed, if something can be simplified, let's do it! |
|
@chrisirhc looking closer at the things I'm not sure it makes sense to try to have those tests running on both 1.2 and 1.0. We should probably create a 1.2-specific branch, but only after BS3 support is out. |
|
@pkozlowski-opensource You're right, it might make more sense to write tests for their expected behavior in 1.2 . |
|
Looks like if I can trust what the unit tests are telling me, ui-bootstrap is currently AngularJS 1.2 compatible. 😄 Ref: angular/angular.js@909cabd , angular/angular.js@27e9340 |
|
@chrisirhc this is totally awesome!!!! It is great to see such good contributions, since those are rare! Once again, thnx for such hard, good quality work. |
|
FWIW, I'm trying to upgrade to BS 3 and NG 1.2 at the same time. I've forked the main bs3 branch here and tried to incorporate many of chrisirhc's fixes. https://github.com/matthughes/bootstrap/tree/bs3ajs12 I would think most users upgrading to BS 3 would also want NG 1.2 at the same time. All tests pass short of date/time pickers; they are disabled until I can spend more time looking at them. |
|
@pkozlowski-opensource Thank you for your kind words. 😄 @matthughes those tests should pass with the fixes here. |
|
@chrisirhc I believe your test fixes were assuming Bootstrap 2.3, no? That's what is on your branch. I'm trying to get both 3.0 BS and 1.2.3 NG working together. |
|
@matthughes They should work on the Bootstrap 3.0 branch as the changes are only on the tests. |
- Ignore other tests for now as they require angular-ui#1342 to be merged
- Ignore other tests for now as they require angular-ui#1342 to be merged
|
Rebased to master, looks good. |
|
@chrisirhc There is already a test helper file at Let's merge this 👍 |
|
Definitively, let's merge it in! I think we should do the next release with the AngularJS1.2.x support. @bekos @chrisirhc could you guys merge this one? |
|
@pkozlowski-opensource I can do. No problem. Should we squash the commits or it is better to see the analytic history? |
|
@chrisirhc Can you rebase to current master, and consider the changes mentioned above? |
|
I was just thinking, that since angularjs has a new release very often now, instead of updating manually the lib & mock files, it is better to use bower for this. WDYT? |
|
I really like the idea of using bower! My only concern is that we forget to check support for previous minor versions. However, this should be okay if we provide a AngularJS version option through grunt that way to allow quickly running tests on an older version. |
|
Bower sounds like a good option - if it works :-) Saw some problems with it in the past, wasn't very reliable so I've abandoned it. It was few moths back so I might be spreading FUD here. Let's try to see what it gives. Any takers? |
|
I can work on it as I rework the changes on this PR. 👍 Should be able to do it by tonight. |
ghost
assigned
|
@chrisirhc That would be awesome 👍 |
|
OK, looked at the bower PR and I think it needs a bit more thinking. At the same time I would be keen to move on on this one and not have it blocked by the build-related discussion. Could we make this one the old-school and focus on bower in #1542? @chrisirhc I can take a stab at merging this one if you are busy |
|
Just checked it quickly, there is only one merge conflict in the datepicker.spec.js |
|
I should be able to do it pretty quickly. Got stuck last night due to mucking around with Bower. |
|
@chrisirhc cool, so I'm not looking into this one in this case. As mentioned there seems to be conflict only in one file. |
- Use unminified version for easier debugging
- Since Angular 1.2 is now using the ng-hide class to hide the element, it needs to be added into the DOM for styles to be computed.
AngularJS 1.2 introduces separate isolate scopes for directives that require isolate scopes. See angular/angular.js@909cabd and angular/angular.js@27e9340
- Use ngBindHtml with $sce as ngBindHtmlUnsafe has been removed See angular/angular.js@dae6947
|
@pkozlowski-opensource , done. There were some other changes needed for the tooltip spec due to new tests added. |
|
@chrisirhc awesome. Just working on merging it with few minor changes (moving testing helpers to one place etc.). |
|
@chrisirhc It is in master now. The only thing I've changed is this one: A W E S O M E work, we've got doors open to including 1.2-specificities ($animate and other improvements!). |
|
Ah, yes sorry about that. I meant to do that merge but I forgot! |
Update: Looks like if I just switch over to AngularJS 1.2 without maintaining 1.0 compatibility, there are little changes to be made and the tests pass.
This pull request is a work in progress.I'm re-working tests so that they test for AngularJS 1.2 compatibility and differences.
Purpose of this:
This pull request:
Should not break any tests while running on AngularJS 1.0.8 (the current test setup)See comments belowTo use:
test-lib/angular.jsandtest-lib/angular-mocks.jswith the 1.2 versions of those files.The biggest change affects datepicker and timepicker tests is that ngModel is no longer using the isolate scope of the element directive, so using
$parent.modelbreaks the tests as it's trying to access the model from the parent of the root scope. See breaking changes in the changelog and the original issue: angular/angular.js#1924 .Another change that's breaking tests is that
ngShowHidenow adds/removes theng-hideclass instead of setting thedisplayCSS property directly.While working on this, I noticed a number of hacks around the edge cases. Some attributes had model-like behavior (two-way binding) (e.g.
open), while other attributes were being picked up on the controller (before the linking and before the scope maybe ready). I think it's important to note these differences. This can be done either through documentation or via a naming convention of the attributes. I noticed an eye icon that I believe is supposed to mean something, but it's not quite clear what that means. (Shall put out another issue for this)