Skip to content
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

Acc Compute v2: Updating the Compute v2 Acceptance Tests #820

Merged
merged 27 commits into from
Mar 21, 2018

Conversation

jtopjian
Copy link
Contributor

For #818

@coveralls
Copy link

coveralls commented Mar 13, 2018

Coverage Status

Coverage remained the same at 73.825% when pulling ccd20d9 on jtopjian:acc-compute-updates into 35000af on gophercloud:master.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 13, 2018

Build failed.

@jtopjian jtopjian changed the title [wip] Acc compute updates Acc compute updates Mar 14, 2018
@jtopjian jtopjian requested a review from jrperritt March 14, 2018 02:59
@jtopjian
Copy link
Contributor Author

jtopjian commented Mar 14, 2018

This is ready for review.

There's a lot going on here, but the changes generally fall under the following categories:

  1. Addition of AssertEquals in various places. There aren't checks for everything but it's a good start and definitely enough to have more confidence in the acceptance testing.

  2. Addition of RequiresX convenience functions: https://github.com/jtopjian/gophercloud/blob/947904130a9773785edd1b726a727839cd2bf27f/acceptance/clients/conditions.go

    These are pretty handy and add some nice-looking declarations to the tests: https://github.com/jtopjian/gophercloud/blob/947904130a9773785edd1b726a727839cd2bf27f/acceptance/openstack/compute/v2/migrate_test.go#L30

    These functions are key to getting the entire acceptance suite into OpenLab.

  3. General cleanup where it was obvious to do so.

  4. Removal of verbose error messages: 9479041

    This was intentionally done as the last commit so I could easily nix it if requested. On one hand, the verbose error messages make the acceptance tests useful reference examples. On the other hand, omitting them makes the tests smaller and easier to write (thus more appealing to contributors).

This PR also includes #819 since I found it handy for my own debugging here.

Happy to make any changes as requested.

/cc @dklyle

edit: given the amount of green/red diffs in each file, I recommend looking at the finished files. They're quite clean and streamlined. :)

@jtopjian jtopjian changed the title Acc compute updates Acc Compute v2: Updating the Compute v2 Acceptance Tests Mar 14, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 14, 2018

Build succeeded.

@jtopjian
Copy link
Contributor Author

Merge conflict has been resolved.

I should have been more clear with my previous comment: I don't expect anyone to review each and every change here. This is a bulk update to the compute acceptance tests and if I was only adding a bunch of AssertEquals statements, I'd be comfortable merging them on my own.

But I'd like to get feedback / signoffs on:

  1. The checks added to conditions.go
  2. The removal of all of the t.Logf statements.

If both of those are acceptable, I'm going to go through the rest of the tests and make similar changes in bulk.

If those items shouldn't be added or need changed, I'll make them here and then proceed with the other tests in a similar fashion.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Mar 15, 2018

Build succeeded.

Copy link
Contributor

@jrperritt jrperritt left a comment

Choose a reason for hiding this comment

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

Yep, this all looks reasonable to me

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.

None yet

3 participants