-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Have remote tests scan all facts. #41
Conversation
known_facts = host['facts'] | ||
break | ||
for fact in known_facts.keys(): | ||
try: | ||
assert (str(known_facts[fact]) in str(row[fact])) | ||
assert (str(known_facts[fact]) in str(row.get(fact))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One observation is that if the row does not have the fact then it will convert to the string 'None'
which won't match. But if the code get updated it may break
>>> str(None)
'None'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why not use == here? It could have the false positive of 'abc' being in 'abcd'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that this would be more resilient to white space changes, when I was originally writing this and hand encoding the config file I think I had some issues with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the 'None' issue, am going to make a change and push it and we can see what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good, left an observation and a question that may require a change.
Now I check for facts not present in the scan and register errors if there are expected facts missing from actual scan. This should circumvent the problem around converting possible |
known_facts = host['facts'] | ||
break | ||
for fact in known_facts.keys(): | ||
missed_facts = known_facts.keys() - row.keys() | ||
if (missed_facts != set()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty lists means False for python:
>>> bool(set())
False
>>> bool(set([1,2,3]))
True
With that we can write cleaner code like:
if missed_facts:
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got me again!
if (missed_facts != set()): | ||
# This means there are facts that we expected, | ||
# but were not collected | ||
for fact in missed_facts: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above if block is not needed the for can be used directly. If the missed_facts is empty no iteration will run.
try: | ||
assert (str(known_facts[fact]) in str(row[fact])) | ||
if fact == 'cpu.bogomips': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to move the if outside the try/catch that is something that won't raise the expected issue and will help others understand what you are really trying to catch here.
fact, | ||
row.get(fact), | ||
known_facts[fact], | ||
)) | ||
scan_errors.append(msg) | ||
if len(scan_errors) != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not on this PR but this line is the same as if scan_errors:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code could be improved a little bit and left some comments.
Also, fixes problem of key errors if expected fact not present in scan, will fail more gracefully with informative error message now. Closes #40
Also, fixes problem of key errors if expected fact not present in scan,
will fail more gracefully with informative error message now.
Closes #40
I ran the remote tests against the vcenter machines as well as against my local machine, and this fixed the key jboss fact key errors I was getting!
Once this is merged I will update jenkins config files so it will run tests against all facts.