Add initial qpc source tests #116

elyezer · 2018-01-16T19:20:12Z

Close #115

kdelee

Looks great. Most of my comments are doc suggestions or things you may consider doing in another PR if you think this one is big enough ;).

Only action item I would request you definitely look into is either stubbing test cases relating to the source and cred "types" or filing an issue to make sure that gets included in a future PR.

kdelee · 2018-01-16T20:02:27Z

camayoc/tests/qcs/cli/utils.py

+
+    :param options: A dictionary mapping the option names and their values.
+        Pass ``None`` for flag options.
+    :param output: A regular expression that matches the expected output.


Can you specify a bit more particularly what format you mean? I think you are saying it just has to be a string, but might be misread as a compiled re object

This actually should be a regular expression pattern string. That is important because it will be compiled as a regular expression and any regex syntax issue will raise an exception that it is not able to compile.

This is related to what output should be conversation we have in one of the previous comments. Let's do the following, find the way output should be (it must be the same for all *_show helpers) and then I will update this properly. Later I will update the cred_show to match the output object like one for source_cred.

kdelee · 2018-01-16T20:08:24Z

camayoc/tests/qcs/cli/test_sources.py

+
+
+@pytest.mark.parametrize('hosts', ('127.0.0.1', '192.168.0.0/24'))
+def test_add_with_cred_hosts_file(


I think it is OK if you just want to target network sources with this PR, but I think you could possibly reuse almost all this code for vcenter and satellite if you parametrize on a tuple or dict like [('vcenter', 'vcenter.example.com'), ('network', 'network.example.com'), ('network', '127.0.0.1'), ('network', 192.168.0.0/24')]

Thank you for the parametrize data, I am going to update this.

kdelee · 2018-01-16T20:14:53Z

camayoc/tests/qcs/cli/test_sources.py

+        '    "name": "{}",\r\n'
+        '    "port": 22,\r\n'
+        '    "source_type": "network"\r\n'
+        '}}\r\n'


I forget where you landed on this last time with a similar issue, but you may consider if this regex can get wrapped up in a constant or constructed by a helper function. Also, you may have mentioned your intention to do this in the chat.

This is something that I still not sure. cred_show accepts a dict and build the string, that is ok because all values for credentials are strings so it is easy to generalize that. On the other hand, source_show has some interesting stuff in its output, it has integer and list also which makes a little harder to generalize.

You can ask, why don't you pass the dict as data and then use json.dumps to generate the output? That is a valid and a great question but the issue I found was to escape JSON structure for regex, like how I would differentiate that [ is meant to match a literal [ or be a regex list expression. Here is where things get interesting and I need to do a little more research if I will be able to do that.

With all those options in the table the last one way to generalize or avoid that repetition would be to have a format string constant for the output (as you suggested). Then call format and pass the specific data. This last option seems to be the one which won't mess with the regex escaping and JSON formatting.

As I said I will continue my research and see if I find another option that we should consider. Let me know if you have something else in your mind.

That last option of having a format string constant seems the most sane!

kdelee · 2018-01-16T20:18:37Z

camayoc/tests/qcs/cli/test_sources.py

+        '}}\r\n'
+        .format(cred_name, hosts, name)
+    )
+


Don't see any tests regarding the "type" of the source. Examples: using the wrong type credential, invalid type for source, changing type of source, or not specifying the type. If you don't want to implement I would suggest at least a stub or issue to remind yourself

This is the missing part, the approach I've used on these changes was to grab what we have for rho profile and use that as a initial coverage. The current tests are pretty much equivalent to rho profile ones.

That said, type is hard coded on this iteration, my next steps will be see if I can parametrize type for the current tests and add some specific to the type option.

I mean, I am just clarifying what I had in mind since you have a really valid point here. Thank you for bringing it up. I will request another round of review after adding those improvements and then you can see if I am missing something. You have a far more experience with the source API than I do so your input is valuable.

kdelee · 2018-01-16T20:22:45Z

camayoc/tests/qcs/cli/test_sources.py

+
+def test_edit_port_negative(isolated_filesystem, qpc_server_config):
+    """Edit the port of a not created source entry.
+


Suggestion: "Edit port of a source entry that does not exist"

Much better, thank you

kdelee · 2018-01-16T20:23:26Z

camayoc/tests/qcs/cli/test_sources.py

+
+def test_clear_negative(isolated_filesystem, qpc_server_config):
+    """Clear a source which is not created.
+


Suggest "Attempt to clear a source that does not exist"

Much better, thank you

coveralls · 2018-01-17T10:03:09Z

Coverage remained the same at 72.993% when pulling 677b7a2 on issues/115 into 73255e5 on master.

elyezer · 2018-01-17T14:40:30Z

@kdelee I am glad I've sent this WIP PR and you were able to do a first round of review, that brought good comments and conversation.

I will update the comments accordingly and will ping you for another round of review. Thanks

elyezer · 2018-01-17T20:43:34Z

@kdelee this is ready for another round of review

elyezer · 2018-01-17T20:45:45Z

camayoc/tests/qcs/cli/test_sources.py

+    qpc_source_edit.close()
+    assert qpc_source_edit.exitstatus == 0
+
+    if new_hosts.endswith('0/24'):


Be aware that edit should not be possible for CIDR and ansible range notations quipucords/quipucords#449. I still have to think how to handle this in this test or create a separated one for that.

Yeah, there are a couple of cases that apply to both vcenter and satellite and that's why I broke some tests in the API side out to "network" and "host_manager" types, so I could treat vcenter and satellite together.

coveralls · 2018-01-17T20:46:41Z

Coverage remained the same at 72.993% when pulling cb4eb0f on issues/115 into 73255e5 on master.

kdelee

Looking good, but I am confused about test_add_with_cred_hosts. If you could clear that up for me we can move forward.

kdelee · 2018-01-17T23:08:56Z

camayoc/tests/qcs/cli/test_sources.py

+
+
+@pytest.mark.parametrize('hosts', ('127.0.0.1', '192.168.0.0/24'))
+def test_add_with_cred_hosts(


I like the parametrized fixture that provides the source type, but as you point out in quipucords/quipucords#449, it is a bug if we can add a vcenter or satellite source with the CIDR notation. I'm not seeing how you are handling that in this test case, maybe I'm missing something.

That is why I was suggesting having groupings in tuples or some other object that paired types with appropriate host values. So you could match satellite and vcenter with both alphabetical DNS names and individual IPv4 addresses, and for network do those as well as CIDER and the 10.10.182.[0:255] range notation. But there are plenty of ways you could address this.

You are right, I am not handling the difference here yet. I found this bug later yesterday and opened the issue and updated the PR with the new code I was able to get done.

For hosts in specific I think another fixture could be created so we can group the source type with the expected hosts that it should work. Also we can create another one with invalid data to make sure we perform a negative test too.

Nice, that would be good, and if you put the conftest.py in a couple levels up, I could probably use the fixture in the API tests as well

kdelee · 2018-01-17T23:13:31Z

camayoc/tests/qcs/cli/test_sources.py

+def test_edit_cred_negative(
+        isolated_filesystem, qpc_server_config, source_type):
+    """Edit the cred of a not created source entry.
+


Edit the cred of a source entry that does not exist

kdelee · 2018-01-17T23:15:08Z

camayoc/tests/qcs/cli/test_sources.py

+
+def test_edit_hosts_negative(
+        isolated_filesystem, qpc_server_config, source_type):
+    """Edit the hosts of a not created source entry.


Edit the cred of a source entry that does not exist

kdelee · 2018-01-17T23:19:08Z

camayoc/tests/qcs/cli/test_sources.py

+    qpc_source_edit = pexpect.spawn(
+        'qpc source edit --name {} --hosts {}'
+        .format(invalid_name, new_hosts)
+    )


If the test is just checking that I can't edit a source that does not exist, I'm not sure if you really need to bother creating a valid source in the first part of this test. Are you just wanting there to be at least on source on the server at the time that you try and edit your fake source? I don't think it hurts anything but may just be extra code we don't need.

This is the negative test I talked about on #116 (comment). This test needs to be update so it can check if hosts can't be edited with invalid values and for that a valid source will be required.

After updating this I will check any other test that may require update in terms of being a good test.

coveralls · 2018-01-19T16:14:30Z

Coverage remained the same at 72.993% when pulling 054b316 on issues/115 into 73255e5 on master.

coveralls · 2018-01-19T16:19:16Z

Coverage remained the same at 72.993% when pulling 1592c25 on issues/115 into 73255e5 on master.

coveralls · 2018-01-19T16:19:16Z

Coverage remained the same at 72.993% when pulling 1592c25 on issues/115 into 73255e5 on master.

coveralls · 2018-01-19T16:41:26Z

Coverage remained the same at 72.993% when pulling 994a1d6 on issues/115 into 8ba60d9 on master.

coveralls · 2018-01-19T16:41:26Z

Coverage remained the same at 72.993% when pulling 994a1d6 on issues/115 into 8ba60d9 on master.

Close #115

elyezer · 2018-01-19T17:16:00Z

@kdelee I think this is now ready for the final review. I've run the tests and got the following:

===================================== 44 passed, 4 xfailed in 135.31 seconds ======================================

coveralls · 2018-01-19T17:17:17Z

Coverage remained the same at 72.993% when pulling 2f5e782 on issues/115 into 8ba60d9 on master.

kdelee · 2018-01-19T17:24:53Z

LGTM, great progress. Let's merge this and move on. I think by next week we could probably get some of these and the API tests running on satellite types too (that don't actually require connecting to the satellite), but that's for another day!

elyezer requested a review from kdelee January 16, 2018 19:20

elyezer changed the title ~~Add initial qpc source tests~~ [WIP] Add initial qpc source tests Jan 16, 2018

kdelee approved these changes Jan 16, 2018

View reviewed changes

elyezer force-pushed the issues/115 branch from 677b7a2 to cb4eb0f Compare January 17, 2018 20:43

elyezer commented Jan 17, 2018

View reviewed changes

kdelee suggested changes Jan 17, 2018

View reviewed changes

elyezer force-pushed the issues/115 branch 2 times, most recently from 054b316 to 1592c25 Compare January 19, 2018 16:14

elyezer force-pushed the issues/115 branch from 1592c25 to 994a1d6 Compare January 19, 2018 16:38

Add initial qpc source tests

2f5e782

Close #115

elyezer force-pushed the issues/115 branch from 994a1d6 to 2f5e782 Compare January 19, 2018 17:15

elyezer changed the title ~~[WIP] Add initial qpc source tests~~ Add initial qpc source tests Jan 19, 2018

kdelee approved these changes Jan 19, 2018

View reviewed changes

elyezer merged commit 2f5e782 into master Jan 19, 2018

elyezer deleted the issues/115 branch January 19, 2018 17:26



		@pytest.mark.parametrize('hosts', ('127.0.0.1', '192.168.0.0/24'))
		def test_add_with_cred_hosts_file(


		def test_edit_port_negative(isolated_filesystem, qpc_server_config):
		"""Edit the port of a not created source entry.


		def test_clear_negative(isolated_filesystem, qpc_server_config):
		"""Clear a source which is not created.



		@pytest.mark.parametrize('hosts', ('127.0.0.1', '192.168.0.0/24'))
		def test_add_with_cred_hosts(

Add initial qpc source tests #116

Add initial qpc source tests #116

Conversation

elyezer commented Jan 16, 2018

kdelee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdelee Jan 16, 2018 • 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

coveralls commented Jan 17, 2018 • edited Loading

elyezer commented Jan 17, 2018

elyezer commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 17, 2018 • edited Loading

kdelee left a comment • 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

coveralls commented Jan 19, 2018 • edited Loading

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018

coveralls commented Jan 19, 2018

elyezer commented Jan 19, 2018

coveralls commented Jan 19, 2018 • edited Loading

kdelee commented Jan 19, 2018

kdelee Jan 16, 2018 •

edited

Loading

coveralls commented Jan 17, 2018 •

edited

Loading

coveralls commented Jan 17, 2018 •

edited

Loading

kdelee left a comment •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading

coveralls commented Jan 19, 2018 •

edited

Loading