Add basic e2e tests for amp-list-load-more #21064
Conversation
cathyxz
force-pushed
the
feature/list-tests
branch
from
50e22fc
to
52a8905
Compare
| const article = await controller.findElement('article'); | ||
| await controller.scroll(article, {top: 10}); | ||
|
|
||
| const sixthItem = await controller.findElement( |
There was a problem hiding this comment.
Is there a better way to wait for load-more to finish?
There was a problem hiding this comment.
I suppose you could also wait for the indicator to go away, then check the number of items (assuming the rendering of items happens at the same time).
There was a problem hiding this comment.
For now waiting for an element to appear that is not initially present is the best way, IMO
| const article = await controller.findElement('article'); | ||
| await controller.scroll(article, {top: 10}); | ||
|
|
||
| const sixthItem = await controller.findElement( |
There was a problem hiding this comment.
I suppose you could also wait for the indicator to go away, then check the number of items (assuming the rendering of items happens at the same time).
| expect(visibility).to.equal('visible'); | ||
| const display = await controller.getElementCssValue(seeMoreButton, | ||
| 'display'); | ||
| expect(display).to.equal('block'); |
There was a problem hiding this comment.
@cvializ Would it be useful to have a simple isDisplayed method here? For example, the approach outlined here: https://w3c.github.io/webdriver/#dfn-bot-dom-isshown
Basically check if something has a rect (therefore has some sort of display, and all the parents do), check the opacity to see if it is actually visible (I think you need to check the ancestors too), check the computed visibility property and optionally check if it is hidden overflow from a parent element.
We could also start with something simple and add to it.
There was a problem hiding this comment.
+1! That would be nice indeed.
There was a problem hiding this comment.
I feel like that could be more than is needed, since our tests should be checking the state of the components they're testing. This feels like a heuristic more than a clear assertion and could introduce flakiness when people use it believing it to be more solid.
For example, is something visible if it meeds all the above criteria but is also position: absolute; left: -9999? What is the author of the code's intention? What if its opacity is .0001 for a technical reason like needing to be picked up by a screen reader?
I think for initial testing / proving it can be implemented as a helper method in test code, then be promoted to a FunctionalTestController API method after it is stable and well-defined. What do you think?
There was a problem hiding this comment.
Sounds good, we can start with just a helper function in this file that does what is needed. For the position: absolute; left: -9999 case, screen readers will not be able to interact with that item, so I think that is also something we will want to check for. This is covered by the "hidden by overflow from a parent element", where the element is positioned in the overflow area of the body (otherwise the body would scroll horizontally).
Perhaps a better function would be isInteractable (which is less vague than isDisplayed), which would not check opacity, since that does not affect whether the element can receive interaction.
| await controller.scroll(article, {top: 10}); | ||
|
|
||
| const sixthItem = await controller.findElement( | ||
| 'div.item:nth-child(4)'); |
|
|
||
| it('should load more items on scroll', async() => { | ||
| let listItems = await controller.findElements('.item'); | ||
| expect(listItems.length).to.equal(2); |
There was a problem hiding this comment.
can you try expect(listItems).to.have.length(2)?
| expect(listItems.length).to.equal(2); | ||
|
|
||
| const loader = await controller.findElement('[load-more-loading]'); | ||
| expect(loader).to.not.be.null; |
There was a problem hiding this comment.
I think you can also do expect(loader).to.be.ok, I seem to get a bit tripped up reading to.not.be.null personally.
| const seeMoreButton = await controller.findElement('[load-more-button]'); | ||
| expect(seeMoreButton).to.not.be.null; | ||
| const visibility = await controller.getElementCssValue(seeMoreButton, | ||
| 'visibility', 'visible'); |
There was a problem hiding this comment.
@cvializ Hmm, getElementCssValue says it has a third arg in the abstract method, but the actual implementation does not. Should we remove the third arg from the abstract method since expect will already wait for it to go the desired value?
| const article = await controller.findElement('article'); | ||
| await controller.scroll(article, {top: 10}); | ||
|
|
||
| const sixthItem = await controller.findElement( |
There was a problem hiding this comment.
For now waiting for an element to appear that is not initially present is the best way, IMO
* First e2e test for amp-list-load-more * Added a load-more items test * Add test for load-more-end * Add basic tests for load-more=auto * Fix spelling, variable names, spaces * to be ok
* First e2e test for amp-list-load-more * Added a load-more items test * Add test for load-more-end * Add basic tests for load-more=auto * Fix spelling, variable names, spaces * to be ok
Adds two simple fixtures respectively for the manual load-more and automatic load-more cases. Tests that each case successfully renders all items and load-more visuals, and successfully triggers a load-more on click or on scroll.