Add Button functional tests.
#168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
=========================================
Coverage ? 99.24%
=========================================
Files ? 18
Lines ? 929
Branches ? 279
=========================================
Hits ? 922
Misses ? 1
Partials ? 6Continue to review full report at Codecov.
|
| 'button should be visible'(this: any) { | ||
| return getPage(this.remote) | ||
| .findByCssSelector('button:first-of-type') | ||
| // as per mwistrand, `isDisplayed` is broken in Safari 10, commented out for now |
There was a problem hiding this comment.
Could we add a link to a Selenium driver issue listed somewhere so we can come back and fix this later?
There was a problem hiding this comment.
I just added the Selenium issue link and fixed the indentation. Please let me know if there's any other issues. Thanks!
|
Yeah. It is the known Safari issue on browserstack for today: |
|
|
||
| 'button should be visible'(this: any) { | ||
| return getPage(this.remote) | ||
| .findByCssSelector('button:first-of-type') |
There was a problem hiding this comment.
Please change all findByCssSelector calls to use the generate css-module class names by importing the appropriate m.css file.
There was a problem hiding this comment.
Thanks for the review Tom. I looked at 6612379 where we changed to use css-module class to access the internals of a widget, and this approach makes sense to me. However, in my case here where the widget structure is simple and I am not trying to exercise the internal components of the widget, but the widget itself - I am trying to locate the first widget on the page (there's several button widgets in the example), so I am not sure if css-module class would be any help locating what I am looking for. For example, by using css-module, it seems I still have to do something like this: .findByCssSelector(`button.${css.root}`:first-of-type). That said, the example page probably should have included ids for each widget for external tests like this one.
There was a problem hiding this comment.
I see what you are saying @lzhoucs, however you are assuming that the tag will be button. This is not an assumption that you can make in all cases. More importantly, these tests are also to help with dojo 2 adopters and it's important that we consistently adopt the right way to deliver our code and tests. As per our css-modules approach, no css selector at any point should ever reference the tagname.
There was a problem hiding this comment.
I agree that tag name shouldn't be used. I've added a wrapper div with id for each widget in the example, together with css-module class to locate the widget. Please take another look when you get chance, @tomdye, thanks!
There was a problem hiding this comment.
Much better, although I'd have used a class over an id. 😉
There was a problem hiding this comment.
I think id is not always bad. In the example page, each example is unique. There will always be one example-1 on the page. And I am not trying to consistently style multiple examples, but access each of them individually.
| 'button should be visible'(this: any) { | ||
| return getPage(this.remote) | ||
| .findByCssSelector('button:first-of-type') | ||
| // `isDisplayed` is broken in Safari 10, commented out for now. See https://github.com/SeleniumHQ/selenium/issues/3029 |
There was a problem hiding this comment.
Rather than leaving this in, can we please remove it in a separate commit? Then it can be added back in again as and when it's appropriate to do so. I think it's better not to have commented out code in our master branch.
There was a problem hiding this comment.
Dylan suggested a slightly different approach: #168 (comment), so that's why the comment is there. I've removed the commented code, however I do have a concern that it might get lost over time once the issue with Safari is fixed.
|
@tomdye , thanks for the review. I've addressed your feedback. Please take another look when you get chance and let me know if there's any other issues. Thanks! |
Type: functional tests
The following has been addressed in the PR:
Resolves #149