-
Notifications
You must be signed in to change notification settings - Fork 1.3k
travis: Fix failing travis tests on main #6152
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
Conversation
|
@blueorangutan package |
|
@blueorangutan package |
| try: | ||
| #Clean up, terminate the created network offerings | ||
| cleanup_resources(self.apiclient, self.cleanup) | ||
| cleanup_resources(self.apiclient, reversed(self.cleanup)) |
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.
You can consider super(TestProjectLimits, self).tearDown()
| try: | ||
| #Cleanup resources used | ||
| cleanup_resources(cls.api_client, cls._cleanup) | ||
| cleanup_resources(cls.api_client, reversed(cls._cleanup)) |
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.
You can consider super(TestProjectLimits, cls).tearDownClass()
DaanHoogland
left a comment
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.
some remarks (and what Wei said)
| with self.assertRaises(Exception): | ||
| PublicIPAddress.create( | ||
| self.apiclient, | ||
| zoneid=virtual_machine_1.zoneid, | ||
| services=self.services["server"], | ||
| networkid=network.id, | ||
| projectid=self.project.id | ||
| ) |
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.
aren't those part of the test purpose; that it throws an exception when trying to create a public IP?
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.
it constantly fails with an Assertion Error stating - no exception raised @DaanHoogland
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.
ok, wonder why this assertion is in there than. If it is supposed to succeed, we are fine.
| for network in networks: | ||
| self.cleanup.append(Network(network.__dict__)) | ||
|
|
||
| self.cleanup.append(self.project_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.
it shouldn't be added to cleanup with so much posibilities of exceptions in between.
| account=self.account.name, | ||
| domainid=self.account.domainid | ||
| ) | ||
| self.cleanup.append(self.project_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.
why shouldn't the project be cleaned?
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.
It is cleaned @DaanHoogland but at a later stage, as first the networks need to be cleaned up. As the tests failed with such errors - Can't delete the project yet because it has 1 Network to clean up
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.
it shouldn't be added to the clean list with other statements inbtween. This means exceptions can occur before it is added. It must be added immediately.
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 errors are because the list isn't processed in reverse. the call to super takes care of 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.
Thanks @DaanHoogland , I'll update the tests with @weizhouapache 's suggestion of using super.
358385d to
aecb021
Compare
weizhouapache
left a comment
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.
code lgtm
travis tests results pass 💯
|
@blueorangutan package |
|
@nvazquez a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2966 |
nvazquez
left a comment
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.
LGTM - no smoke tests needed, Travis tests are good now thanks @Pearl1594
Description
This PR addresses #6115
Prior fix:
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?