Fix and Re-enable some unit tests on dialog, tooltip, and draggable #1144
Conversation
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
|
fwiw, I see Tooltip tests failed in the CI build as well. In my environment that passes just fine. I suspect that is the later phantomjs v1.9.2 that I'm using vs 1.8.2-2 in your CI build. The other CI failures were expected as mentioned. In my build: |
|
Interesting, I can't speak to the history of exactly why these tests were disabled. I verified that the Despite the focus tests working, I'm still getting several dialog and tooltip failures in my environment. I'm not sure why we would have different results. I'm also using version 1.9.2 of phantomjs. |
|
If you drop this file https://github.com/bdowling/grunt-lib-phantomjs/blob/570fce49f9ca0be04230f693d98c637d9404c0a8/phantomjs/main.js |
|
Ah ok. After doing that all tests pass for me as well. This look great. Could you ping us when your PR is included in a released version of grunt-lib-phantomjs so we can look into moving forward with this? |
|
@bdowling I've reviewed gruntjs/grunt-lib-phantomjs#47, see my comments there. |
| @@ -56,7 +56,7 @@ | |||
| }, | |||
| "dependencies": {}, | |||
| "devDependencies": { | |||
| "grunt": "0.4.1", | |||
| "grunt": "~0.4.2", | |||
There was a problem hiding this comment.
Can you change this to "0.4.2"? We like using exact version numbers.
There was a problem hiding this comment.
Actually, why is this change required?
|
@bdowling thanks again for contributing! Can you split this into different PRs? One for the |
|
@bdowling I'd like to land what we can. Can you split this as @mikesherov suggested? |
|
Sure, I will take a look, I was hoping to see the grunt phantomjs fix go in so I could mark the release # dependency on that side at the same time. |
While working on one of my own modules that extends dialogs, I wanted to validate existing dialog tests passed and realized they were disabled in grunt/phantomjs out of the box.
When looking deeper, it seems the main reason my have been the tiny viewportSize phantomjs starts out with causing some of the issues. A PR to gruntjs/grunt-lib-phantomjs#21 adds the ability to change the viewport directly from the config. I know the tests won't pass in your CI until this is fixed, but I wanted to record this PR just so I didn't forget about them. Alternatively, I could have included alternate phantomjs/main.js and a phantomScript option, but that didn't seem like the best option.
Note, I do have questions about the fixes I did here (bdowling/jquery-ui@jquery:master...master#diff-1b4b30b16776f6b3f0a4f05ddfbeb518R561).. Not sure why
$(":focus")differs fromdocument.activeElementin these instances in phantomjs, but I don't think changing this impacts what is being tested here. Perhaps a separate test that should be included in jQuery itself?