search

ibm-js / deliteful

Multichannel (desktop/mobile) UI Custom Elements Library
http://ibm-js.github.io/deliteful
Other
70 stars 36 forks source link

Combobox: finish keyboard tests #503

Open wkeese opened 9 years ago

wkeese commented 9 years ago

179f96dcc2f59b8417352dde0402260d5df14dff (and the subsequent commits) are missing tests for a lot of features that are in the code:

AdrianVasiliu commented 9 years ago

Pushed via cf618e11104fc1703b302049d567d5a812cc46d7.

This adds test cases for various things (for keyboard navigation but also for mouse navigation in addition to what is tested already in the unit test). Among the points mentioned in the present issue, it tests:

  • closing the dropdown by selecting a value via ENTER key (selectionMode=single) and similar test for selectionMode=multiple. besides checking that the dropdown was closed, it should check that the "change" event was fired
  • closing the dropdown via ESC;
  • auto-scrolling (both up and down) due to up / down arrow keys
  • SPACE key in selectionMode=single and autoFilter=true should leave the drop down open and refine the search
  • do not close popup when clicking a category item (pointer test, not a keyboard test)

Remarks about some points:

  • test for the case when "keynav-child-navigated" returns something that isn't a render item; I don't know when that would ever happen in real life, but since you have code for it (removing aria-activedescendant etc.) you should be testing for it.

The handling of this case was for being on the safe side, but I agree this can't happen in real life at least with the current code of KeyNav and List, and it might hide a bug elsewhere. So I removed this code.

  • closing the dropdown via ESC; presumably in this case it should revert to the original selection. and there should be no change event.

Browsers (Chrome, FF, IE11 / Win) do not revert the state of a native HTML <select> when closing via ESC. Combobox goes the same way. I agree this is debatable.

  • selectionMode=single: the tests for up/down arrow keys should check that aria-activedescendant is correctly set,

I realize now that I missed it, I'll add this later.

  • auto-scrolling (both up and down) due to up / down arrow keys

Added some basic testing, but I'll try to review/add more. On the other side, #498 may move the feature to List.

  • selectionMode=single: the tests for up/down arrow keys should check that [...] the current List item (and only that item) is marked as selected (via a CSS class)
  • similarly, the selectionMode=multiple tests should check that the selected list items (and only those items) have the CSS class for being selected

Combobox uses the standard API for selecting list items. IMO, the addition/removal of CSS classes for selected items is a mechanism entirely owned by List, which is out of the scope of Combobox tests.

  • ENTER key while on category should not close the drop down (selectionMode=single)

List does not allow to navigate to a category item (either using keyboard or mouse). So you can't press ENTER while on a category.

AdrianVasiliu commented 9 years ago

I see errors on some browsers (Firefox) in the travis build that I didn't get in my testing with test:remote. I reopen for now because of that, and because of the few additions to tests that I plan to do.

AdrianVasiliu commented 9 years ago

I see errors on some browsers (Firefox) in the travis build

Errors only in FF. These appear to go away if I upgrade the FF version number in our deliteful intern.js from 31 to the latest available on Saucelabs: 33 (while I locally have FF 35). Question: can I upgrade the test config to FF33?

cjolif commented 9 years ago

We are supposed to support FF31 which is the latest ESR release so I hardly see how we can update FF version without increasing the risk to miss a specific problem?

AdrianVasiliu commented 9 years ago

Okay, so I'll dig into the issue with FF31.

AdrianVasiliu commented 9 years ago

These are the FF 31.x ESR versions: image

I have downgraded FF on my Win7 to either the oldest (31.0ESR) or the newest (31.4.0ESR). In both cases the Combobox functional test passes fine with grunt test:local... I'll keep trying to guess why it fails on sauce, but here's where I am...

AdrianVasiliu commented 9 years ago

Okay, found another change which makes the test pass: upgrading the selenium version (for the Firefox test config). Setting explicitly "selenium-version": "2.43.0" for the FF config makes FF 31.0 ESR makes the test pass with grunt test:remote. According to https://docs.saucelabs.com/reference/test-configuration/, Selenium 2.43.0 is the most recent supported version, while the default is 2.40.0.

I noticed that, with our current confg:

{ browserName: "firefox", version: "31", platform: [ /*"OS X 10.6", "Linux", */ "Windows 7" ],
            name : "deliteful"}

the test output says firefox 31.0 on XP (instead of Windows 7 as we'd expect...), while with

{ browserName: "firefox", version: "31", platform: [ /*"OS X 10.6", "Linux", */ "Windows 7" ],
            "selenium-version": "2.43.0", name : "deliteful"}

the output refers more generically to firefox 31.0 on WINDOWS

Now, I wanted to check that not only Combobox tests pass with this newer selenium version, but also all other unit and functional tests. However, my attempts to check it fail apparently because I get randomly disconnected - while running just the Combobox tests is quick enough and passes fine.

To avoid loosing more time on it, if are you okay with the principle of this selenium-version upgrade, I'd do the change then rely on travis builds to make sure it goes well (if needed, it's easy to backtrack the selenium-version change).

AdrianVasiliu commented 9 years ago

No luck, some tests (Combobox, List) fail on Firefox in the travis build with ERROR Selenium Server stopped responding. I see people complaining about various combinations of selenium driver and FF versions, e.g. http://stackoverflow.com/questions/25646639/firefox-webdriver-doesnt-work-with-firefox-32. I'm not sure why it did work for me with grunt test:remote (what is the difference that matters). I try with selenium 2.42.0, just in case...

AdrianVasiliu commented 9 years ago

Selenium 2.42.0 didn't do the trick either, but then I tried 2.41.0 and this worked fine! That is all tests passed in Firefox (firefox 31.0 on XP: 0/475 tests failed (0 skipped) - https://travis-ci.org/ibm-js/deliteful/builds/50631845).

Fine, but... While no test is marked failed on any platform, we get in this build, for the Safari / iOS 7.1 platform:

UnknownError: [POST http://(redacted)@localhost:4444/wd/hub/session / {"desiredCapabilities":{"platformName":"iOS","platformVersion":"7.1",
"browserName":"safari","deviceName":"iPhone Simulator","appium-version":"1.2.2","name":"deliteful", [...] ] 
The Sauce VMs failed to start the browser or device

As far as I can see, this would be an issue unrelated with both the Combobox test additions and the upgrade of selenium version for the Firefox platform.

Seeing that the appium version currently indicated by Saucelabs' platform configurator for iOS 7.1 is 1.3.4 (https://docs.saucelabs.com/reference/platforms-configurator/#/) while we still use 1.2.2, I gave a try to the upgrade of appium-version for the iOS config. No luck, this made 12 test cases to fail on iOS... To make more painful, my attempts to test with grunt test:remote broke today most of the time with timeout at tunnel creation. Just in case, I restarted this latest travis build.

AdrianVasiliu commented 9 years ago

Status update: The attempt to upgrade appium-version for the iOS config not being successful (even after a restart of the travis build), I reverted back to the initial appium-version: 1.2.2. So the repo now contains (in comparison with the state at the latest green travis build):

In these conditions, the travis build results (https://travis-ci.org/ibm-js/deliteful/builds/50919556) are as follows:

FAILED Test  'main - List tests - list-prog-5.html' on internet explorer 11 on WINDOWS:
ScriptTimeout: Polling timed out with no result
UnknownError: [POST http://(redacted)@localhost:4444/wd/hub/session / {"desiredCapabilities":{"platformName":"iOS","platformVersion":"7.1",
"browserName":"safari","deviceName":"iPhone Simulator","appium-version":"1.2.2","name":"deliteful","tunnel-identifier":"1424081462395","idle-timeout":60,"build":"b3ed9f0299d868262264bfdab28ef346949f002e"}}] 
The Sauce VMs failed to start the browser or device

Again, as far as i can see, all these errors are (or seem to be) unrelated with both my Combobox test additions, and with my changes in tests/intern.js (since the only remaining change is in the FF config which passes fine). Obviously, the trouble with the iOS test platform does need to be solved, but this would be unrelated with my work on Combobox.

edchat commented 9 years ago

After this commit: https://github.com/ibm-js/deliteful/commit/daab7113d3a4462b74ced8996578e5f9f48cb959 Travis still failed, but it was because the test run exceeded 50 minutes. But it seems like the iOS tests may have stopped, the last iOS test was "Test 'main - ProgressBar - Invalid init value' on iOS on MAC 8.1: " The iOS 8.1 tests run successfully for me.

AdrianVasiliu commented 9 years ago

Okay, thanks. So appium 1.3.4 also gives test failures (that you solved by remote.skip()) on iOS 8.1 - I guess pretty much the same failures that I got with appium 1.3.4 on iOS 7.1. For sure, it's better to try getting the iOS 8.1 tests pass rather than iOS 7.1 - even if having them pass on both 7.1 and 8.1 would be even nicer. Not sure if we can do something to get them pass on iOS without the radical remote.skip solution... Maybe waiting for the next appium-version is the thing to do ;-)

edchat commented 9 years ago

Well I was not able to get appium 1.3.4 with iOS 7.1 to run at all, the tests would not start for me, but they ran with iOS 8.1.

edchat commented 9 years ago

I restarted the travis test and it passed this time. I agree skipping the tests is not ideal, but at this point there are 52 tests being skipped on iOS, so skipping these 5 is probably no worse than the other 40+. I did verify that the StarRatings are clickable on the iOS 8.1 simulator.

AdrianVasiliu commented 9 years ago

Well I was not able to get appium 1.3.4 with iOS 7.1 to run at all, the tests would not start for me

Strange, it did start for me: https://travis-ci.org/ibm-js/deliteful/builds/50648487 (but with failures in various tests, as you got with iOS 8.1).

Anyway, we now have a green build (https://travis-ci.org/ibm-js/deliteful/builds/51526511)! (thanks to the skipping of failing tests on iOS 8.1).

wkeese commented 9 years ago

Great, sounds like you guys have figured out versions of selenium and appium to make all the tests pass. So this ticket is just left open for a few remaining Combobox test updates? IIUC those are:

• selectionMode=single: the tests for up/down arrow keys should check that aria-activedescendant is correctly set,

AdrianVasiliu commented 9 years ago

Yes, you summarized correctly what I wrote in https://github.com/ibm-js/deliteful/issues/503#issuecomment-73921075.