Skip to content
This repository has been archived by the owner on Feb 7, 2023. It is now read-only.

Improve error messages in tests #290

Merged
merged 2 commits into from
Dec 12, 2017

Conversation

mike-nguyen
Copy link
Collaborator

@mike-nguyen mike-nguyen commented Dec 12, 2017

This is the follow on to #288 to address #239. #288 addressed the
error messages in the roles and this commit addresses the error
messages in the tests themselves.

This is the follow on to projectatomic#288 to address projectatomic#239.  projectatomic#288 addressed the
error messages in the roles and this commit addresses the error
messages in the tests itself.
@mike-nguyen
Copy link
Collaborator Author

bot, retest this please

@mike-nguyen
Copy link
Collaborator Author

I just realized that this is going to run every test we have and most likely it will timeout/fail.

msg: |
Expected: Error message should indicated the deployment is already in
the unlocked state: development
Actual: double_unlock.stderr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need double curly braces here? It probably works without them, but adding them would be consistent elsewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that one! I think it would actually print out the literal double_unlock.stderr

msg: |
Expected: {{ g_invalid_pkg }} is not currently requested in rpm-ostree
uninstall output
Actual: {{ uninstall.stderr }}
when: "'\\'{{ g_invalid_pkg }}\\' is not currently requested' not in uninstall.stderr"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside unrelated to the changes here, this conditional looks ugly as sin. I wonder if we can improve it....

msg: "/var/lib/containers/atomic/{{ g_hw_name }}/rootfs does not exist"
msg: |
Expected: /var/lib/containers/atomic/{{ g_hw_name }}/rootfs exists is True
Actual: /var/lib/containers/atomic/{{{ g_hw_name }}/rootfs exists is {{ hw.stat.exists }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra opening curly brace

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised Ansible didn't choke on this when I tested the test

msg: "Initramfs args are incorrect. Expected /etc/rpmostree-file. Actual {{ ros_booted['initramfs-args'][1] }}"
msg: |
Expected: /etc/rpmostree-file in rpm-ostree initramfs-args
Actual: {{ ros_booted['initramfs-args'][1] }}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra double quote at the end of the line

@miabbott
Copy link
Collaborator

Looks good overall. Just a couple of nits and we can probably merge this in.

@mike-nguyen
Copy link
Collaborator Author

Just pushed a fixup. Thanks for the review! Ansible seems a little looser when you use the | so I need to be a little more careful. The tests didn't complain when I ran them.

@miabbott
Copy link
Collaborator

None of the CI failures are due to the change itself, so this is good to merge. Thanks for continuing the cleanup!

@miabbott miabbott merged commit 6ad66c9 into projectatomic:master Dec 12, 2017
@mike-nguyen mike-nguyen deleted the err_msg_tests branch December 13, 2017 17:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants