Skip to content

Remove 'remove-raise-on-failure' calls from integration_tests#788

Merged
OddBloke merged 2 commits into
canonical:masterfrom
TheRealFalcon:remove-raise-on-failure
Jan 26, 2021
Merged

Remove 'remove-raise-on-failure' calls from integration_tests#788
OddBloke merged 2 commits into
canonical:masterfrom
TheRealFalcon:remove-raise-on-failure

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Pycloudlib no longer raises exceptions when cloud-init fails to start,
and the API has been updated accordingly. Changes have been made to
integration tests accordingly

Proposed Commit Message

Remove 'remove-raise-on-failure' calls from integration_tests

Pycloudlib no longer raises exceptions when cloud-init fails to start,
and the API has been updated accordingly. Changes have been made to
integration tests accordingly

Additional Context

canonical/pycloudlib#93

Test Steps

pytest tests/integration_tests

Tested on both lxd containers and lxd vms. Failures were unrelated.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

Pycloudlib no longer raises exceptions when cloud-init fails to start,
and the API has been updated accordingly. Changes have been made to
integration tests accordingly
Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

One docstring update needed (plus an unrelated observation). I'm going to test this locally before I Approve.

Comment thread tests/integration_tests/instances.py Outdated
Comment on lines 62 to 64
This wraps pycloudlib's `BaseInstance.restart` to pass
`raise_on_cloudinit_failure=False` to `BaseInstance.wait`, mirroring
our launch behaviour.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This needs updating.

@@ -27,13 +27,13 @@ def _detect_reboot(instance: IntegrationInstance):
# detecting the first boot or second boot, so we also check
# the logs to ensure we've booted twice. If the logs show we've
# only booted once, wait until we've booted twice
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Total aside, but I just noticed that this test could fail (by exiting early) if run on an instance which has booted more than once without cloud-init.log removal: the "first" boot would have multiple entries. Generally for this sort of check, I think we need a "before" count which is expected to increase before we consider the reboot complete (and this is why we should capture this in library code, so every .restart() call DTRT).

Anyway, moving on.

@OddBloke
Copy link
Copy Markdown
Collaborator

Tested this locally so LGTM; +1 once the docstring update happens.

Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Thanks!

@OddBloke OddBloke merged commit a9c904d into canonical:master Jan 26, 2021
@TheRealFalcon TheRealFalcon deleted the remove-raise-on-failure branch June 29, 2021 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants