-
Notifications
You must be signed in to change notification settings - Fork 165
[1LP][RFR] Enhanced security group test case for multiple checks #9886
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
view.paginator.set_items_per_page(item_count) | ||
assert len(view.entities.get_all()) <= view.paginator.items_per_page | ||
view.paginator.set_items_per_page(1000) | ||
total_count = len(view.entities.get_all()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question:
- Why are you setting
set_items_per_page
to 1000? if it is for getting total count then please check if there is any other way available to get total count of entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can refer "view.entities.get_all(surf_pages=True)"
to surf the page for entites to get total count.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a method for this in PaginationPane
- https://github.com/ManageIQ/integration_tests/blob/master/widgetastic_manageiq/__init__.py#L1809
It will be slow to use get_all if there are actually 1000 items. It'll also not get the total count if there are more than 1000.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mshriver!
updated with your suggestion
8f663af
to
401f249
Compare
401f249
to
9abea7d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
Purpose or Intent
Enhanced as per suggestion given: https://github.com/ManageIQ/integration_tests/pull/9799/files#r366572190
PRT Run
{{ pytest: cfme/tests/cloud_infra_common/test_relationships.py::test_change_network_security_groups_per_page_items --use-provider complete --use-template-cache -qsvv }}