feat(components/lists): add paging test harnesses #3185
Conversation
Blackbaud-CoreyArcher
added
the
risk level (author): 1
|
View your CI Pipeline Execution ↗ for commit 21d1ff4.
☁️ Nx Cloud last updated this comment at |
| it('should get the paging component by data-sky-id', async () => { | ||
| const { pagingHarness } = await setupTest({ dataSkyId: 'other-paging' }); | ||
|
|
||
| await expectAsync(pagingHarness.getPagingContent()).toBeRejected(); |
There was a problem hiding this comment.
Can we confirm that it's getting the other-paging component with something other than a toBeRejected which could be true even from that component failing to build? My vote is on pagingLabel so u wont have to build the stuff inside paging and the component can stay one line in the template.
There was a problem hiding this comment.
The dataSkyId filter already confirms it's the correct paging component. It will fail the test if it doesn't find that specific one. The rejected getPagingContent method also confirms that it's the paging component that doesn't have a sky-paging-content child
| async #buttonIsDisabled(button: TestElement): Promise<boolean> { | ||
| const disabled = await button.getAttribute('disabled'); | ||
| return disabled !== null; | ||
| } |
There was a problem hiding this comment.
Missing a function to get the pagingLabel input value that is the aria-label for the nav
| const disabled = await button.getAttribute('disabled'); | ||
| return disabled !== null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we add a function to say if paging is in the "waiting" mode since we add that and show consumers how to set it up in the content code examples
There was a problem hiding this comment.
I wouldn't expect consumers to want or need to test that the paging component is in a waiting state while they are in the process of handling a contentChange event. We don't do that in other places where we incorporate our own Wait.
| await button.click(); | ||
| } | ||
|
|
||
| public async getCurrentPage(): Promise<number> { |
There was a problem hiding this comment.
missing jsdocs
|
|
||
| /** | ||
| * A set of criteria that can be used to filter a list of `SkyPagingHarness` instances. | ||
| * @internal |
There was a problem hiding this comment.
should this be internal?
|
|
||
| /** | ||
| * Harness to interact with a page control element in tests. | ||
| */ |
There was a problem hiding this comment.
Since consumers dont set the page number text and only know it as the 'page number' and not 'page control text', i think we should mark this as internal and use the SkyPagingHarness to have functions to just get getActivePageNumber and clickPageNumber etc
There was a problem hiding this comment.
I don't like this approach because it's not immediately clear how they should interact with the controls if they're obfuscated behind the paging harness. Do they ask to click a specific page number, which may not be in the list? Do they ask to click the Nth page control, and if so how do we enforce there are only that many controls to begin with? Giving them an array of the paging controls that exist feels very intuitive for them exploring the structure of the page to me. They get, say, 5 test harness elements, they can do a sanity check that they are paging in the way they expect, and interact with them the same way a user would.
| 'Could not click the next button because it is disabled.', | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
can we add a code example spec file for the basic one as well? I know we havent been being stringent about that but we should moving forward imo
There was a problem hiding this comment.
I didn't make one because the basic code example isn't being used. If we decide to change that, we can add it.
| @@ -102,7 +102,7 @@ export class DemoDataService { | |||
| totalCount: people.length, | |||
| }).pipe( | |||
| // Simulate network latency. | |||
| delay(1000), | |||
| delay(100), | |||
There was a problem hiding this comment.
why are we reducing this?
There was a problem hiding this comment.
I was worried about this delay causing the sample tests I was writing to time out. 100ms is still a pretty good length of delay.
AB#2195510