[1LP][RFR] Fix test_sdn_nsg_firewall_rules, update to wrapanapi changes. #7616
Conversation
jarovo
changed the title
jarovo
changed the title
digitronik
changed the title
cfme/networks/security_group.py
Outdated
| return [self.instantiate(name=s.name) for s in list_networks_obj] | ||
|
|
||
| collection = (self.instantiate(name=s.name) for s in list_networks_obj) | ||
| return [a for a in collection if apply_filters(a, self.filters)] |
There was a problem hiding this comment.
Required:
I am suspect It will not return security group objects when I will call without a filter.
There was a problem hiding this comment.
You are not right:
In [4]: def apply_filters(item, filters):
...: for attr_name, val in filters.items():
...: if getattr(item, attr_name) != val:
...: return False
...: return True
...:
In [5]: class A(object):
...: def __init__(self, foo):
...: self.foo = foo
...:
In [7]: collection = [A(str(i)) for i in range(10)]
In [13]: [a.foo for a in collection if apply_filters(a, {})]
Out[13]: ['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']
In [14]: [a.foo for a in collection if apply_filters(a, {'foo': '3'})]
Out[14]: ['3']
There was a problem hiding this comment.
As you stated below, this apply_filters method here is usable beyond just this class, and I think has a lot of potential if we place it in BaseCollection.
That being said, I think we need to carefully look at your intended use of this application of the filter. Note that above here, the parent filter is already applied to the navigation, so if the collection is one where the parent is a provider, then this method will only get entities that are listed in the UI filtered by that provider. This all method, and every collection all method, should have a consistent way to apply any collection filters to both the navigation and entity filtering.
I suspect this is working around either a use of the collection where the parent/provider filter has been applied but more items were included that expected.
Looking at the test below, I'm even more sure that's what is happening - because the filter in the test is for 'provider', but this all method is navigating using parent filter, which navigates via the provider's navigation destination and applies the filter in the UI in MIQ.
Can you split the creation of this method into a separate PR, and concentrate this PR on resolving test fixtures, after we discuss my comment above about this being a workaround? Test fix may just be to apply a 'parent' filter, or to change this all method to navigate based on provider filter.
jarovo
changed the title
digitronik
changed the title
| view = navigate_to(secgroup, 'Details') | ||
|
|
||
| assert 'Port' == view.entities.firewall_rules[0][3].text | ||
| assert '22' == view.entities.firewall_rules[1][3].text | ||
| assert 'Port' == view.entities.firewall_rules[1][3].text |
There was a problem hiding this comment.
Why the change here to which row is indexed? There needs to be an in-line comment explaining these 'magic numbers' at the minimum, so we're not guessing at what row should have been there.
There was a problem hiding this comment.
Bit bigger discussion about consistently applying filters in all methods, and I like the direction you were going with that. But I'd like to separate a test failure that may just be related to an incorrectly applied filter from the enhancement that includes a new BaseCollection method.
mshriver
changed the title
* Do network provider refresh. It should be refreshed to get new port rules. * Move port rule params to the test case * address issues in wrapanapi PR #7596 * Fix filtering in security_group
|
@mshriver @digitronik The apply_filters method was renamed to match_items and is being added in #8018. |
jarovo
changed the title
| assert '22' == view.entities.firewall_rules[2][3].text | ||
| else: | ||
| # The table has two header rows. We cannot access the second one with | ||
| # widgetastic. So let's hope the column of index 3 is the Port Range |
There was a problem hiding this comment.
Let's get an issue written against wt.core for handling two header tables
Purpose or Intent
Fixing test_sdn_nsg_firewall_rules. It is currently not doing network provider refresh which is incorrect. It should be refreshing to get new port rules.
Updating tests to new wrapanapi changes.
{{ pytest: -v cfme/tests/networks/test_sdn_inventory_collection.py -k test_sdn_nsg_firewall_rule --use-provider azure }}