[1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add #10033

mmojzis · 2020-04-01T17:15:00Z

Also added needed views and create function.

jarovo · 2020-04-02T20:00:45Z

cfme/tests/networks/test_sdn_addresses.py

+@pytest.fixture()
+def network_manager(appliance, provider):
+    network_managers = appliance.collections.network_providers.filter({"provider": provider})
+    yield network_managers.all()[0]


This is bit suspicious to me. How is it ensured the first returned network_manager is the one the test wants? Is it because you assume all other providers will get removed and only the OpenStack will be ensured to be present?

Well, how can openstack provider have multiple network managers? Never seen this option in CFME.
Also we are using the same fixture for storage managers and it works well. : https://github.com/ManageIQ/integration_tests/blob/master/cfme/tests/storage/test_manager.py#L31

jarovo · 2020-04-02T20:23:43Z

cfme/tests/networks/test_sdn_subnets.py

+
+
+@pytest.fixture()
+def network_manager(appliance, provider):


This method does the same as the one in the other file, so I am frowning on picking the first network_manager here as well.

But I think also having this a fixture doesn't provide enough (any) benefits yet it just creates some potential problems in future because it is a code dup. It is fixture for effectively a one-liner. I am sure one-liners dups are not considered a problem most of the times.

I will ask other reviewers what they think about fixtures like this one.

Well, in the future when they add another providers with network managers the fixture makes sense.

@JaryN valid concern for sure.

I wouldn't consider this a blocker to merging this PR, but warrants some future enhancement.

This could be a globally available fixture (cfme/fixtures/), or even a subcollection of the parent cloud provider.

jarovo · 2020-04-02T20:24:54Z

cfme/tests/networks/test_sdn_subnets.py

+    view = subnets_collection.create_view(SubnetView)
+    view.flash.assert_message(
+        "Unable to create Cloud Subnet: Invalid input for cidr. Reason: '{}' is not a valid IP "
+        "subnet.".format(invalid_cidr))


Interesting that Travis didn't complain/converted this to f-string

What is f-string?

This is f-string.

my_identifier = "baz" f"foo {my_identifier} bar" ``

jarovo · 2020-04-02T20:25:03Z

cfme/tests/networks/test_sdn_subnets.py

+    view = subnets_collection.create_view(SubnetView)
+    view.flash.assert_message(
+        "Unable to create Cloud Subnet: Invalid input for cidr. Reason: '{}' is not a valid IP "
+        "subnet.".format(invalid_cidr))


Interesting that Travis didn't complain/converted this to f-string

Changed fstring.

jarovo · 2020-04-03T12:40:14Z

cfme/tests/networks/test_sdn_addresses.py

+
+@pytest.fixture()
+def network_manager(appliance, provider):
+    network_managers = appliance.collections.network_providers.filter({"provider": provider})


Suggested change

network_managers = appliance.collections.network_providers.filter({"provider": provider})

network_manager, = appliance.collections.network_providers.filter({"provider": provider}).all()

yield network_manager

This way, when there are more than one network_provider, we will get an error. This is a good thing as it will notify us that something is not as we did expect.

In [6]: a, = (b for b in range(1)) In [7]: a Out[7]: 0 In [8]: a, = (b for b in range(2)) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-8-c3f22dcf86ee> in <module> ----> 1 a, = (b for b in range(2)) ValueError: too many values to unpack (expected 1)

Sorry, I don't understand the code.

The code is using Pythons value unpacking to make sure the value we get is only one in the iterable.

You can do this with tuples:

a, b, c = 1, 2, 3

which assigns a with 1 and b with 2, c with 3

Of course ths works with single-item tuples:

a, = [1]

this assings a with 1. But it has the advantage of failing when more items are being unpacked:

a, = 1,2

will fail with ValueError: too many values to unpack (expected 1)

This works for any iterable, including generators. So when you are tempting to do
my_variable = some_iterable[0]
and you assume only single item is available in some_variable, it is good to do
my_variable, = some_iterable
instead, because it will fail when the assumption is not met.

Okay, got it. Thanks. Changed as you proposed.

john-dupuy · 2020-04-09T13:38:19Z

@JaryN are you okay to move this one to 1LP?

jarovo

Sorry for the delay.

jawatts · 2020-04-24T14:46:35Z

cfme/networks/floating_ips.py

+                   'fixed_ip_address': fixed_ip_address,
+                   'cloud_tenant': tenant})
+        view.add.click()
+        view.flash.assert_success_message(f'Floating IP "{floating_ip_address}" created')


We want to assert_no error() here instead and have the actual flash message asserted in a test.

Agreed @jawatts, @mmojzis can you cover this flash assertion with a positive CRUD test case? Looks like only negative test cases are included in this PR.

jawatts

One small comments but otherwise looks good!

mshriver · 2020-04-24T19:06:45Z

cfme/tests/networks/test_sdn_addresses.py

+
+@pytest.fixture()
+def network_manager(appliance, provider):
+    network_manager, = appliance.collections.network_providers.filter({"provider": provider}).all()


Can you wrap this potential ValueError in a pytest.skip call, so it doesn't start throwing errors when there's more/less than 1 element in the list.

@mshriver Is this something like you wanted? https://github.com/ManageIQ/integration_tests/pull/10033/files#diff-d0ee99cb7213e3f233490166c76aab34R15

mshriver

Two small changes requested.

For the positive flash message, I'd accept this PR with just a flash.assert_no_error in the create method and no positive flash message text assertion.

mmojzis · 2020-04-27T13:27:31Z

Two small changes requested.

For the positive flash message, I'd accept this PR with just a flash.assert_no_error in the create method and no positive flash message text assertion.

@jawatts @mshriver
These tests are only negative. It should check the format of flash message when invalid cidr is filled in.
I added these tests only because there was qe_test_coverage needed for these BZs.
I understand that you also want positive test that passes, but I didn't have qe_test_coverage for that.
I have no more time to write positive test.

dajoRH · 2020-04-27T13:30:47Z

I detected some fixture changes in commit 18e93fb

The local fixture network_manager is used in the following files:

cfme/tests/networks/test_sdn_subnets.py
- test_network_subnet_invalid_cidr

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

mmojzis changed the title ~~Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 1, 2020

dajoRH added lint-ok WIP-testing labels Apr 1, 2020

mmojzis force-pushed the invalid_address_tests branch from 45c0bf4 to 79d8344 Compare April 2, 2020 13:08

mmojzis changed the title ~~[WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 2, 2020

dajoRH removed the WIP-testing label Apr 2, 2020

jarovo reviewed Apr 2, 2020

View reviewed changes

jarovo reviewed Apr 3, 2020

View reviewed changes

mmojzis force-pushed the invalid_address_tests branch from 79d8344 to 5c64fb1 Compare April 7, 2020 11:43

dajoRH changed the title ~~[RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [WIP]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 7, 2020

dajoRH added needs-lint WIP and removed lint-ok labels Apr 7, 2020

mmojzis force-pushed the invalid_address_tests branch from 5c64fb1 to 601268d Compare April 7, 2020 18:25

Added invalid cidr/ip tests for floating_ip_add and subnet_add

601268d

dajoRH added lint-ok and removed needs-lint labels Apr 7, 2020

mmojzis changed the title ~~[WIP]Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 7, 2020

dajoRH removed the WIP label Apr 7, 2020

john-dupuy added the new-test-or-feature label Apr 9, 2020

jarovo approved these changes Apr 23, 2020

View reviewed changes

jarovo changed the title ~~[RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [1LP][RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 23, 2020

jawatts self-assigned this Apr 24, 2020

jawatts reviewed Apr 24, 2020

View reviewed changes

jawatts approved these changes Apr 24, 2020

View reviewed changes

mshriver reviewed Apr 24, 2020

View reviewed changes

mshriver suggested changes Apr 24, 2020

View reviewed changes

mshriver changed the title ~~[1LP][RFR]Added invalid cidr/ip tests for floating_ip_add and subnet_add~~ [1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add Apr 24, 2020

dajoRH added the WIP-testing label Apr 24, 2020

wrap network_manager get from collections for invalid_cidr tests

18e93fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add #10033

[1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add #10033

mmojzis commented Apr 1, 2020

jarovo Apr 2, 2020 •

edited

Loading

mmojzis Apr 3, 2020

jarovo Apr 2, 2020 •

edited

Loading

mmojzis Apr 3, 2020

mshriver Apr 24, 2020

jarovo Apr 2, 2020

mmojzis Apr 3, 2020

jarovo Apr 3, 2020

jarovo Apr 2, 2020

mmojzis Apr 7, 2020

jarovo Apr 3, 2020

mmojzis Apr 3, 2020

jarovo Apr 3, 2020

mmojzis Apr 7, 2020

john-dupuy commented Apr 9, 2020

jarovo left a comment

jawatts Apr 24, 2020

mshriver Apr 24, 2020

jawatts left a comment

mshriver Apr 24, 2020

mmojzis Apr 27, 2020

mshriver left a comment

mmojzis commented Apr 27, 2020

dajoRH commented Apr 27, 2020

	network_managers = appliance.collections.network_providers.filter({"provider": provider})
	network_manager, = appliance.collections.network_providers.filter({"provider": provider}).all()
	yield network_manager



		@pytest.fixture()
		def network_manager(appliance, provider):

[1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add #10033

Are you sure you want to change the base?

[1LP][WIPTEST]Added invalid cidr/ip tests for floating_ip_add and subnet_add #10033

Conversation

mmojzis commented Apr 1, 2020

jarovo Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarovo Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-dupuy commented Apr 9, 2020

jarovo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawatts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshriver left a comment

Choose a reason for hiding this comment

mmojzis commented Apr 27, 2020

dajoRH commented Apr 27, 2020

jarovo Apr 2, 2020 •

edited

Loading

jarovo Apr 2, 2020 •

edited

Loading