aws / aws-parallelcluster-ui

Apache License 2.0
31 stars 17 forks source link

Paginate list clusters #307

Closed judysng closed 4 months ago

judysng commented 4 months ago

Changes

How Has This Been Tested?

Deployed in personal account, created and deleted clusters Wrote and ran unit tests

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

gmarciani commented 4 months ago

Observations on testing this type of change

The change looks good to me, but the test with 1 cluster does not fully exercise this change as pagination is not used in that case.

To exercise the change there are the following ways:

  1. create more cluster than the usual page size: not worth it because by default the page size is hundreds of elements and we do not want to create hundreds of clusters for this test.
  2. reduce the default page size to 1 element (temporarily, only for testing purposes) and create 2 clusters. Better but it would imply changes on PCAPI side. This would be great, but it's costly because the page size for CFN operations is not based on the number of elements, but on the size of the returned JSON (1MB per age, see doc)
  3. Unit test with mocked PCAPI.

Solution 3 is what I would recommend to validate this change. Regarding unit tests, let's have a look at the existing unit tests and check if we can cover this change with reasonable time. If you think that lot of effort would be required to add this test, let's proceed with this change and address this test gap in a follow up activity.

gmarciani commented 4 months ago

Thanks for adding the unit tests even if there was not boilerplate to take advantage from :-)