-
Notifications
You must be signed in to change notification settings - Fork 75
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
test(network): add behave tests to ensure network errors are handled #3272
test(network): add behave tests to ensure network errors are handled #3272
Conversation
PR ChecklistHow to use this checklistHow to use this checklistPR AuthorFor each section, check a box when it is true. PR ReviewerCheck that the PR checklist action did not fail. Bug ReferencesNone. Confirm
How to properly reference fixed bugs
Test UpdatesUnit Tests
Integration Tests
Documentation
Does this PR require review from someone outside the core ubuntu-pro-client team?
|
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.
Looks good 💯 , some minor nits
81a332a
to
745dc59
Compare
okay ready for re-review now 🫡 @dheyay |
745dc59
to
0b91f75
Compare
@a-dubs seems |
8cceb87
to
657d0a7
Compare
Tests have been constrained to only xenial and noble since the functionality being tested by this is unlikely to break and only needs tested on the oldest and newest LTS releases. |
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.
Looks good to me, would just need to squash commits before merging
66f33d2
to
28907b0
Compare
took the liberty to rebase this after merging #3267 - I'll take a last look and probably merge it |
And I attempt to attach `contract_token` with sudo | ||
Then stderr contains substring: | ||
""" | ||
Failed to attach machine. See https://ubuntu.com/pro/dashboard |
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 this actually needs to be fixed for us to consider the respective LP bug closed. The problem here is that you don't have internet connection:
- there is no indication that lack of connection was the problem
- how are people supposed to access their dashboard if the network is down?
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.
@a-dubs if you want, we can land the tests and then you change this message on a next step?
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.
Oh shoot yeah you're right. That's not exactly a very useful or specific message. So you're right we definitely need to update the message to consider those bugs closed. As such, I think it would be best if I put this back in draft state and make the changes to the source code such that the source code changes and tests are bundled together into one logical commit, so that when I say "fixes lp #XXXX", I mean fixes
features/network_failures.feature
Outdated
Examples: ubuntu release | ||
| release | machine_type | | ||
| xenial | lxd-container | | ||
| noble | lxd-container | |
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.
See how this test is similar to the upper one, except for the service to be disabled?
You can create another column in the table below with "service to disable" so it looks like this:
Examples: ubuntu release | |
| release | machine_type | | |
| xenial | lxd-container | | |
| noble | lxd-container | | |
Examples: ubuntu release | |
| release | machine_type | service | | |
| xenial | lxd-container | livepatch | | |
| xenial | lxd-container | realtime-kernel | | |
| noble | lxd-container | livepatch | | |
| noble | lxd-container | realtime-kernel | |
Feel free to ignore this though - if you think this is good as is, keep it.
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.
At first glance, I was initially onboard because i love reducing duplicate code where reasonable, but then I noticed the and stdin 'y\ny'
that only exists in the realtime kernel tests and not the livepatch tests.
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.
good catch forget what I said
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.
okay i am unable to forget and got the following working but its a bit jank and not sure how to feel about it:
Scenario Outline: Network errors for enabling Realtime kernel and Livepatch are handled gracefully
# This test simulates network failure by disabling internet connection
# and then trying to enable realtime-kernel or livepatch
Given a `<release>` `<machine_type>` machine with ubuntu-advantage-tools installed
When I attach `contract_token` with sudo and options `--no-auto-enable`
Then the machine is attached
Then I verify that `<service>` is disabled
When I disable any internet connection on the machine
And I verify that running `pro enable <service>` `with sudo` <exit_with_optional_stdin_clause>
Then stderr contains substring:
"""
Failed to connect to https://contracts.canonical.com/v1/contracts/
"""
Then I verify that `<service>` is disabled
# Realtime kernel is not supported on LXD containers so we must use a VM
Examples: ubuntu release
| release | machine_type | service | exit_with_optional_stdin_clause |
| xenial | lxd-vm | realtime-kernel | and stdin `y\ny` exits `1` |
| noble | lxd-vm | realtime-kernel | and stdin `y\ny` exits `1` |
| xenial | lxd-vm | livepatch | exits `1` |
| noble | lxd-vm | livepatch | exits `1` |
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.
Instead, would using --assume-yes
on the enable call allow you to avoid the and stdin
so that they can be the same for livepatch and realtime?
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.
Oh you can do that??? That would be way better!
28907b0
to
f94ce30
Compare
f94ce30
to
b03a6a1
Compare
@dheyay @renanrodrigo this should be ready for review now. let's just get this in and then i'll work on actually improving the exception handling and messaging for networking 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.
testing it good
Why is this needed?
This PR adds functionality that ensures that the various networking exception handling issues that have been raised on various LP bugs, which were resolved during the work for #2647 was completed, are properly caught, handled, and raised.
Test Steps
Run the new behave tests locally: