(test): add e2e tests for autocomplete (#4491) #9077
Conversation
Ann40a
requested review from
crisbeto,
jelbourn,
josephperrott and
kara
as code owners
googlebot
added
the
cla: yes
Ann40a
force-pushed
the
autocomplete-e2e
branch
from
5b6da05 to
da57ea2
Compare
|
Please, help me, whom should I add to CODEOWNERS? Should it really be myself as I did or someone from your team? |
.github/CODEOWNERS
Outdated
| @@ -129,6 +129,7 @@ | |||
|
|
|||
| # E2E app | |||
| /e2e/* @jelbourn | |||
| /e2e/components/autocomplete-e2e.spec.ts @ann40a | |||
There was a problem hiding this comment.
Can you mark crisbeto as the code owner here as he is the code owner for the component.
|
|
||
| class AutocompletePage { | ||
| constructor() { browser.get('/autocomplete'); } | ||
| autocompleteInput = () => element(by.id('autocomplete-input')); |
There was a problem hiding this comment.
Personally, I don't know that its strictly necessary to have a class to manage retrieving the elements as you have here. If you would like to keep the class, rather than assigning these are attributes which are functions, can you use typical method declaration?
class AutocompletePage {
constructor() {
browser.get('/autocomplete');
}
autocompleteInput() {
return element(by.id('autocomplete-input'));
}
...
}
There was a problem hiding this comment.
Agreed. I don't think we have that many tests where setups like this start making sense.
There was a problem hiding this comment.
Personally, I prefer to user page objects, because it separates the test logic and elements selection. I've seen this pattern in menu-e2e.spec.ts and decided that it is acceptable to use it. I should remove this class, right?
There was a problem hiding this comment.
We haven't been super consistent about when we use page objects (IIRC only the menu tests have them). Let's keep it for now and we can decide later on whether we want to remove it or switch all of the other e2e tests to use page objects as well.
| @@ -104,7 +104,7 @@ export function getMatAutocompleteMissingPanelError(): Error { | |||
| 'aria-autocomplete': 'list', | |||
| '[attr.aria-activedescendant]': 'activeOption?.id', | |||
| '[attr.aria-expanded]': 'panelOpen.toString()', | |||
| '[attr.aria-owns]': 'autocomplete?.id', | |||
| '[attr.aria-owns]': '(autocomplete && autocomplete._isOpen) ? autocomplete.id : null', | |||
There was a problem hiding this comment.
Can you move this logic into a getter method on the class and then use the attribute in the host binding?
There was a problem hiding this comment.
I remember having issues with NVDA not reading out some options when I tried doing this for mat-select. That being said, I couldn't hear any difference when I did it for autocomplete. Still it might be better to do this in a separate PR since this one is primarily for the tests.
There was a problem hiding this comment.
I did this, because I had received ARIA attributes must conform to valid values error from axe-protractor plugin. Do I understand properly, that I need to revert this change, create a ticket for the error and wait until it is done? Or it is ok to ignore this error, because, as I see now, after rebase on master the number of axe errors increased.
There was a problem hiding this comment.
I think that we can still do it, but it should be done as a separate PR, because it needs a unit test and this PR is primarily focused on adding e2e tests to the autocomplete.
|
|
||
|
|
||
| describe('autocomplete', () => { | ||
| const autocompletePanelSelector = '.mat-autocomplete-panel.mat-autocomplete-visible'; |
There was a problem hiding this comment.
The .mat-autocomplete-visible here shouldn't be necessary, the only time an autocomplete is in the DOM and doesn't have that class is when there are no results.
|
|
||
| class AutocompletePage { | ||
| constructor() { browser.get('/autocomplete'); } | ||
| autocompleteInput = () => element(by.id('autocomplete-input')); |
There was a problem hiding this comment.
Agreed. I don't think we have that many tests where setups like this start making sense.
| @@ -104,7 +104,7 @@ export function getMatAutocompleteMissingPanelError(): Error { | |||
| 'aria-autocomplete': 'list', | |||
| '[attr.aria-activedescendant]': 'activeOption?.id', | |||
| '[attr.aria-expanded]': 'panelOpen.toString()', | |||
| '[attr.aria-owns]': 'autocomplete?.id', | |||
| '[attr.aria-owns]': '(autocomplete && autocomplete._isOpen) ? autocomplete.id : null', | |||
There was a problem hiding this comment.
I remember having issues with NVDA not reading out some options when I tried doing this for mat-select. That being said, I couldn't hear any difference when I did it for autocomplete. Still it might be better to do this in a separate PR since this one is primarily for the tests.
Ann40a
force-pushed
the
autocomplete-e2e
branch
from
da57ea2 to
3f629c8
Compare
| @@ -0,0 +1,11 @@ | |||
| <section> | |||
There was a problem hiding this comment.
One thing that's not clear in the code by itself is that we're trying to move away from standalone e2e test pages. Instead, new e2e tests are based on the examples we publish to material.angular.io. Check out #5735 for an example of adding e2e tests for cards based on the published examples.
|
Closing since this seems pretty stale now. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.