test.unit.cli.test_ringbuilder should consisently use run_srb

Bug #1656440 reported by clayg on 2017-01-13
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Wishlist
Nick Miethe

Bug Description

We have some awesome test infrastructure in the RunSwiftRingBuilderMixin - we should use it more!

https://github.com/openstack/swift/blob/b90f2d7a2332b59a21f3e97a1136036a0e097319/test/unit/cli/test_ringbuilder.py#L39

Places where we manually make lists of args and manually pass them to ringbuilder.main and expect them to return 0 should *definitely* be rewritten to use run_srb

e.g.

https://github.com/openstack/swift/blob/b90f2d7a2332b59a21f3e97a1136036a0e097319/test/unit/cli/test_ringbuilder.py#L309

Should just be:

    self.run_srb('create 6 3.14159265359 1')

I think we should also update the signature of run_srb to be:

    status, out, err = self.run_srb

and maybe drop assertSystemExit in favor of

    self.assertEqual(EXIT_SUCCESS, self.run_srb('some args')[0])

Or maybe even better add some asserts around the output in addition to just the exit code!

Nick Miethe (miethe) wrote :

I agree that run_srb removes unnecessary code of making lists of args and passing to ringbuilder.main and its usage would help prevent fat-fingering.

Changed in swift:
assignee: nobody → Nick Miethe (miethe)
status: New → In Progress
status: In Progress → New
status: New → Confirmed
status: Confirmed → In Progress
clayg (clay-gerrard) wrote :

To be 100% clear, this is not a trivial mechanical textual translation (no valuable refactor ever is). I need someone to read/understand the test module and put on their thinking cap and *design* a better pattern.

Specifically - right now run_srb is a *good* helper; but it only supports the use-case of about 50% of the tests that might consume it. There are some tests that only need to make assertions on the *output* of the ring-builder - but others that need to make assertions on the exit code. Right now run_srb returns (stdout, stderr) - so it doesn't support half the potential callers - so they end up having to re-invent half of what run_srb is already good at. Not great!

We could really use some help to fix run_srb so that it can support >50% (90%!?) of the tests - changing to return signature will require updating all of the call-sites of the helper.

But when finished there will be a beautiful, easy to maintain and expand test module - that is *consistently* applying a very powerful test infrastructure interface! Good luck!

Fix proposed to branch: master
Review: https://review.openstack.org/437587

Change abandoned by Nick Miethe (<email address hidden>) on branch: master
Review: https://review.openstack.org/435122
Reason: Taking a different approach at the fix

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers