Merge lp://qastaging/~justin-fathomdb/nova/test-openstack-api-servers into lp://qastaging/~hudson-openstack/nova/trunk

Proposed by justinsb
Status: Work in progress
Proposed branch: lp://qastaging/~justin-fathomdb/nova/test-openstack-api-servers
Merge into: lp://qastaging/~hudson-openstack/nova/trunk
Prerequisite: lp://qastaging/~justin-fathomdb/nova/justinsb-openstack-api-volumes
Diff against target: 657 lines (+458/-29)
7 files modified
nova/api/openstack/ratelimiting/__init__.py (+13/-5)
nova/image/fake.py (+108/-0)
nova/tests/integrated/api/client.py (+40/-0)
nova/tests/integrated/integrated_helpers.py (+77/-22)
nova/tests/integrated/test_keys.py (+2/-1)
nova/tests/integrated/test_servers.py (+216/-0)
nova/tests/integrated/test_volumes.py (+2/-1)
To merge this branch: bzr merge lp://qastaging/~justin-fathomdb/nova/test-openstack-api-servers
Reviewer Review Type Date Requested Status
Nova Core security contacts Pending
Review via email: mp+51089@code.qastaging.launchpad.net

Description of the change

Basic unit testing of the OpenStack API for /servers.

To post a comment you must log in.
Revision history for this message
Rick Harris (rconradharris) wrote :
Download full text (3.2 KiB)

Overall this looks good.

The known_bugs addition looks pretty nifty (but I worry a bit about the added complexity of it). The fake image service looks like a really nice addition.

Below are a few femto-nits:

658 + # Not supported till Python 2.7 / 3.1
659 + #@unittest.skipIf(
660 + # 'server_create_metadata_throws_unhashable_type' in KNOWN_BUGS,
661 + # 'KNOWN_BUG:server_create_metadata_throws_unhashable_type')

If we're going to go this route, perhaps we could roll our own decorator which the TestRunner can use to skip the relevant tests. This would make adoption of this under 2.6 just as easy as 2.7+.

Something along the lines of:

@tests_known_bug("my_known_bug")
def test_my_known_bug(self):
    self.assert_(False)

586 +# print driver.LoggingVolumeDriver.all_logs()
587 +#
588 +#
589 +# create_actions = driver.LoggingVolumeDriver.logs_like(
590 +# 'create_volume',
591 +# id=created_volume_id)
592 +# print create_actions
593 +# self.assertEquals(1, len(create_actions))
594 +# create_action = create_actions[0]
595 +# self.assertEquals(create_action['id'], created_server_id)
596 +# self.assertEquals(create_action['availability_zone'], 'nova')
597 +# self.assertEquals(create_action['size'], 1)
598 +#
599 +# export_actions = driver.LoggingVolumeDriver.logs_like(
600 +# 'create_export',
601 +# id=created_server_id)
602 +# self.assertEquals(1, len(export_actions))
603 +# export_action = export_actions[0]
604 +# self.assertEquals(export_action['id'], created_server_id)
605 +# self.assertEquals(export_action['availability_zone'], 'nova')
606 +#
607 +# delete_actions = driver.LoggingVolumeDriver.logs_like(
608 +# 'delete_volume',
609 +# id=created_server_id)
610 +# self.assertEquals(1, len(delete_actions))
611 +# delete_action = export_actions[0]
612 +# self.assertEquals(delete_action['id'], created_server_id)

Was this left in on purpose? If so, it probably should have a TODO with it.

> 509 + def test_get_servers(self):
> 510 + """Simple check that listing servers works"""
> 511 + servers = self.api.get_servers()
> 512 + for server in servers:
> 513 + print("server: %s" % server)

Can we put an assert in here? Perhaps just a assertNotRaises or something.

> 334 + raise exception.Error("No way to create an image through API??")

This line (and a few others) need i18n treatment, the _("blah").

> 628 + #if found_server['status'] != 'deleting':
> 629 + # break

Left in by accident, or does that need a TODO/FIXME?

> 129 + raise exception.NotFound

Might be helpful to pass in a string with the Exception which includes the Id of the image not found. Something like

  raise exception.NotFound(_("Image %(image_id)s not found) % locals())

> 489 +import time

Should be moved up to top section of imports (per HACKING)

> 641 + #TODO(justinsb): This is FUBAR

Agree wit...

Read more...

Revision history for this message
Soren Hansen (soren) wrote :

What's the status of this? There are concrete comments here, but the branch hasn't been touched for more than a week. Perhaps just set it back to Work in Progress?

Revision history for this message
justinsb (justin-fathomdb) wrote :

Soren: The branch has got unmerged pre-reqs. Here be dragons.

Revision history for this message
justinsb (justin-fathomdb) wrote :

Dragons slayed. There are still unmerged pre-reqs though, so reviewers please focus on those first!

Unmerged revisions

732. By justinsb

PEP8 fun

731. By justinsb

Finally figured out how to work around the latest flip-flops on what the image service should be returning

730. By justinsb

More changing of print -> LOG.debug

729. By justinsb

Removed known_bugs based on community opinion. Commented out the test cases that otherwise won't work.

728. By justinsb

Merged with head, fixing up bazaar's pathetic attempts at conflict resolution.

727. By justinsb

PEP8 fix

726. By justinsb

Need spurious import to pull in flags

725. By justinsb

Scale up rate limits for tests massively, to effectively turn them off

724. By justinsb

More careful checking of servers detail vs non-detail views

723. By justinsb

Add integrated unit tests for server metadata. Also add KNOWN_BUGS list, so that we can test bugs separately from fixing them

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.