Fix 1144 (Fix and Re-enable some unit tests on dialog, tooltip, and draggable) #1173
Conversation
|
Thank you for yet another update. I just landed your grunt-lib-phantomjs PR. I can publish that to npm, so we have to wait for that: gruntjs/grunt-lib-phantomjs#52 Once that is done, I can update grunt-contrib-qunit, if necessary, and then we can get rid of the custom phantomjs script here. |
|
@bdowling if you rebase this on top of master, you can drop the custom phantom script and specify the page options anyway. I got the necessary updates for grunt-contrib-qunit and grunt-lib-phantomjs published. |
I noticed some unit tests were disabled in phantomjs. dialog and draggable depend on a larger viewPort. tooltip just seemed to work, so I reenebaled that as well. Changing the viewportSize depends on a not yet merged feature in gruntjs/grunt-lib-phantomjs#21
Note, this depends on the new phantomjs/main.js in the not yet merged feature in gruntjs/grunt-lib-phantomjs#21
|
@jzaefferer awesome. I updated this branch. (Edited: I realized the fix was in grunt-contrib-qunit v0.4.0 so I repushed) Thanks everyone! |
|
Sorry, its v0.4.0 (0.5.0 was grunt-lib-phantomjs). It should install just fine with |
| // TODO except for all|index|test, try to include more as we go | ||
| return !( /(all|index|test|dialog|tooltip)\.html$/ ).test( file ); | ||
| }) | ||
| files: expandFiles( "tests/unit/**/*.html" ).filter(function( file ) { |
There was a problem hiding this comment.
Could you fix the indent here and the following lines?
|
This looks good to me. Any thoughts on the test fixes, @scottgonzalez? |
|
fwiw, @tjvantoll commented on the :focus stuff here #1144 (comment) |
|
Does anyone know what it is about this test inside Phantom that causes it to fail? "focus": function( elem ) {
return elem === document.activeElement && (!document.hasFocus || document.hasFocus()) && !!(elem.type || elem.href || ~elem.tabIndex);
},I won't claim to understand why :focus is this complex ;) This should probably be addressed separately, but that is why I found changing the test worked.. As I mentioned in other pr, this probably should be fixed the right way in Phantom and have unit tests upstream. |
|
The changes to the individual assertions look fine to me, though there's a spacing problem on the first change. |
|
@scottgonzalez can you point out which spacing is an issue? I think I was asked to put the spaces inside the parens in #1144 if that is what you mean? |
| @@ -361,9 +361,9 @@ test("ensure dialog keeps focus when clicking modal overlay", function() { | |||
| var element = $( "<div></div>" ).dialog({ | |||
| modal: true | |||
| }); | |||
| ok( $(":focus").closest(".ui-dialog").length, "focus is in dialog" ); | |||
| equal( $(document.activeElement ).closest( ".ui-dialog" ).length, 1, "focus is in dialog" ); | |||
|
I squashed the commits and updated the commit message. Code changes were good to go. Thanks @bdowling! |
This PR replaces #1144.
Note that it did not seem feasible to separate the issues. Fixing the :focus ones would be a NOP because the dialog tests still could not be enabled because they would fail on the viewPort issue.
So instead I locally fixed the viewpPort issue by specifying a phantomScript option to qunit and including the (minimally modified) version that supports options.page.Cleaned up to use the now published grunt-lib-phantomjs v0.5.0 via grunt-contrib-qunit v0.4.0 with needed featureHopefully this works.