Fix and update installer ping test

[Griffin-Sullivan]

Problem Statement

This test is failing on stream because hammer ping now has a couple new lines that are nested for the cache

Solution

Simplify the assertion so it's much easier.

Related Issues

[Griffin-Sullivan]

trigger: test-robottelo
pytest: tests/foreman/installer/test_installer.py::test_positive_check_installer_hammer_ping

[lhellebr]

This is a weaker assertion. For example, when I stop the foreman services, I get this output and the assertion won't fail for it:

Error: Failed to open TCP connection to <FQDN>:443 (Connection refused - connect(2) for "<FQDN>" port 443)

[Griffin-Sullivan]

Hmmm good catch. I'll reevaluate.

[Griffin-Sullivan]

@lhellebr during local testing I found that the command won't execute if services are unreachable / down. This causes the test to fail and give you the output you pasted above. So I think it should work fine the way it is.

[Griffin-Sullivan]

trigger: test-robottelo
pytest: tests/foreman/installer/test_installer.py::test_positive_check_installer_hammer_ping

[lhellebr]

What you're trying to do goes against the sense of this test. Think about it, what is the purpose of the second assertion? It will only evaluate if return code of the previous command is 0.
You state the command won't execute if services are unreachable, I think what you mean is it executes and returns non-zero status.
Now, point of this test is testing hammer ping, not only to the level of it returning 0 when it should but also parsing its output and having some expectations on it. The original version didn't do it ideally but it was still a stronger assertion than the new version. If you conclude that "if return value is 0 and there is not a single line saying Status: ok means this still should pass", then you can almost remove the second assertion entirely.

[rmynar]

I think that the purpose of this change is mainly to parse the new output of hammer ping. For hammer ping we have separate tests (https://github.com/SatelliteQE/robottelo/blob/master/tests/foreman/cli/test_ping.py)

[lhellebr]

That being said, you're the owner of the test so feel free to merge without my ACK if you feel I'm being too harsh :)

[Griffin-Sullivan]

I was unaware of the other ping test. I'm not sure I understand the reason we have both tests. I feel inclined to remove this test since it's just a different version of the other test in test_ping.py. Thoughts @rmynar since you did the eval for this file?

[rmynar]

I was also unaware of the other ping test when I did the eval. However I think this one is kind of sanity test for the Installer component (i.e. "No need to continue further testing when some service fails to run after installation/upgrade"). I'm not against removing this test or moving the assertion into another testcase. Current changes seem sufficient to adapt the test to the new hammer ping output.