Add test coverage for BZ:2192939 #14025
Conversation
Gauravtalreja1
added
QETestCoverage
Gauravtalreja1
force-pushed
the
closeloop-2192939
branch
2 times, most recently
from
1f1ff97 to
d2f8dcc
Compare
|
trigger: test-robottelo |
|
trigger: test-robottelo |
Signed-off-by: Gaurav Talreja <[email protected]>
Gauravtalreja1
force-pushed
the
closeloop-2192939
branch
from
d2f8dcc to
ecfe0cf
Compare
|
trigger: test-robottelo |
Co-authored-by: Shubham Ganar <[email protected]>
* Add test coverage for BZ:2192939 Signed-off-by: Gaurav Talreja <[email protected]> * Add :parametrized: keyword to test metadata Co-authored-by: Shubham Ganar <[email protected]> --------- Signed-off-by: Gaurav Talreja <[email protected]> Co-authored-by: Shubham Ganar <[email protected]> (cherry picked from commit 97d9117)
* Add test coverage for BZ:2192939 Signed-off-by: Gaurav Talreja <[email protected]> * Add :parametrized: keyword to test metadata Co-authored-by: Shubham Ganar <[email protected]> --------- Signed-off-by: Gaurav Talreja <[email protected]> Co-authored-by: Shubham Ganar <[email protected]> (cherry picked from commit 97d9117)
* Add test coverage for BZ:2192939 Signed-off-by: Gaurav Talreja <[email protected]> * Add :parametrized: keyword to test metadata Co-authored-by: Shubham Ganar <[email protected]> --------- Signed-off-by: Gaurav Talreja <[email protected]> Co-authored-by: Shubham Ganar <[email protected]> (cherry picked from commit 97d9117)
| @@ -1023,12 +1023,14 @@ def configure_puppet(self, proxy_hostname=None): | |||
| self.execute('/opt/puppetlabs/bin/puppet agent -t') | |||
There was a problem hiding this comment.
Nowadays (since Puppet 6) if you want to bootstrap you use puppet ssl bootstrap. Then you can sign it. You may want to pass --waitforcert 0 as well to prevent it waiting for the server to sign it.
| # This particular puppet run would create the host entity under | ||
| # 'All Hosts' and let's redirect stderr to /dev/null as errors at | ||
| # this stage can be ignored. | ||
| result = self.execute('/opt/puppetlabs/bin/puppet agent -t 2> /dev/null') |
There was a problem hiding this comment.
Using -t isn't quite a good test. While testing the original BZ I noticed it is NOT a reproducer for the issue.
-t (or --test) is a short hand for multiple options. Quoting the man page:
* --test:
Enable the most common options used for testing. These are 'onetime',
'verbose', 'no-daemonize', 'no-usecacheonfailure', 'detailed-exitcodes',
'no-splay', and 'show_diff'.
The --verbose part always creates entries in the report. If you only want to run puppet it should be sufficient to use puppet agent --onetime --no-daemonize --no-splay.
| client.configure_puppet(proxy_hostname=sat.hostname, run_puppet_agent=False) | ||
| # Run either 'puppet agent --detailed-exitcodes' or 'systemctl restart puppet' | ||
| # to generate Puppet config report for host without any messages | ||
| assert client.execute('/opt/puppetlabs/bin/puppet agent --detailed-exitcodes').status == 0 |
There was a problem hiding this comment.
This starts a daemon. You want to use --onetime --no-daemonize --no-splay as arguments to prevent that.
| timeout=300, | ||
| delay=30, | ||
| ) | ||
| report = sat.cli.ConfigReport.list({'search': f'host~{client.hostname}'}) |
There was a problem hiding this comment.
This is a list of reports while the name implies it is a singular entity. I'd name it reports
| ) | ||
| report = sat.cli.ConfigReport.list({'search': f'host~{client.hostname}'}) | ||
| assert len(report) | ||
| assert report[0]['origin'] == 'Puppet' |
There was a problem hiding this comment.
This will pass if there a report with the origin puppet. How about asserting every report has an origin instead?
assert None not in [report['origin'] for report in reports]| ) | ||
| assert len(report) | ||
| assert report[0]['origin'] == 'Puppet' | ||
| facts = session_puppet_enabled_sat.cli.Fact.list({'search': f'host~{client.hostname}'}) | ||
| assert len(facts) |
There was a problem hiding this comment.
This is a rather useless assertion. There's no guarantee that the puppetmaster_fqdn fact is present.
| ) | ||
| assert len(report) | ||
| assert report[0]['origin'] == 'Puppet' | ||
| facts = session_puppet_enabled_sat.cli.Fact.list({'search': f'host~{client.hostname}'}) |
There was a problem hiding this comment.
Why not search specifically for the puppetmaster_fqdn fact?
| assert [f for f in facts if f['fact'] == 'puppetmaster_fqdn'][0][ | ||
| 'value' | ||
| ] == puppet_proxy.hostname |
There was a problem hiding this comment.
I'd write this as:
assert [f['value'] for f in facts if f['fact'] == 'puppetmaster_fqdn'] == [puppet_proxy.hostname]This won't fail with an IndexError if the puppetmaster_fqdn fact isn't present.
* Add test coverage for BZ:2192939 Signed-off-by: Gaurav Talreja <[email protected]> * Add :parametrized: keyword to test metadata Co-authored-by: Shubham Ganar <[email protected]> --------- Signed-off-by: Gaurav Talreja <[email protected]> Co-authored-by: Shubham Ganar <[email protected]>
Problem Statement
Missing test coverage for BZ:2192939, which verifies config report generated for host gets Puppet origin correctly, when reports are without any messages.
Solution