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.

Sign up for GitHub

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

Jump to bottom

Refactor tests from full-stack => unit #6598

Merged
merged 1 commit into from Nov 30, 2016
Merged

Refactor tests from full-stack => unit #6598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
137 changes: 27 additions & 110 deletions cmd/juju/application/deploy_test.go
View file
Open in desktop
Expand Up @@ -12,10 +12,8 @@ import (
"os"
"path"
"path/filepath"
"reflect"
"sort"
"strings"
"sync"

"github.com/juju/errors"
"github.com/juju/gnuflag"
Expand Down Expand Up @@ -1394,27 +1392,27 @@ func (s *DeployUnitTestSuite) TestDeployBundle_OutputsCorrectMessage(c *gc.C) {
// sharpened, this will become so as well.
type fakeDeployAPI struct {
DeployAPI
*callMocker
*jujutesting.CallMocker
}

func (f *fakeDeployAPI) IsMetered(charmURL string) (bool, error) {
results := f.MethodCall(f, "IsMetered", charmURL)
return results[0].(bool), typeAssertError(results[1])
return results[0].(bool), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) SetMetricCredentials(service string, credentials []byte) error {
results := f.MethodCall(f, "SetMetricCredentials", service, credentials)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) Close() error {
results := f.MethodCall(f, "Close")
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) ModelGet() (map[string]interface{}, error) {
results := f.MethodCall(f, "ModelGet")
return results[0].(map[string]interface{}), typeAssertError(results[1])
return results[0].(map[string]interface{}), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) Resolve(cfg *config.Config, url *charm.URL) (
Expand All @@ -1428,7 +1426,7 @@ func (f *fakeDeployAPI) Resolve(cfg *config.Config, url *charm.URL) (
return results[0].(*charm.URL),
results[1].(csclientparams.Channel),
results[2].([]string),
typeAssertError(results[3])
jujutesting.TypeAssertError(results[3])
}

func (f *fakeDeployAPI) BestFacadeVersion(facade string) int {
Expand All @@ -1438,7 +1436,7 @@ func (f *fakeDeployAPI) BestFacadeVersion(facade string) int {

func (f *fakeDeployAPI) APICall(objType string, version int, id, request string, params, response interface{}) error {
results := f.MethodCall(f, "APICall", objType, version, id, request, params, response)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) Client() *api.Client {
Expand All @@ -1453,12 +1451,12 @@ func (f *fakeDeployAPI) ModelUUID() (string, bool) {

func (f *fakeDeployAPI) AddLocalCharm(url *charm.URL, ch charm.Charm) (*charm.URL, error) {
results := f.MethodCall(f, "AddLocalCharm", url, ch)
return results[0].(*charm.URL), typeAssertError(results[1])
return results[0].(*charm.URL), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) AddCharm(url *charm.URL, channel csclientparams.Channel) error {
results := f.MethodCall(f, "AddCharm", url, channel)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) AddCharmWithAuthorization(
Expand All @@ -1467,171 +1465,89 @@ func (f *fakeDeployAPI) AddCharmWithAuthorization(
macaroon *macaroon.Macaroon,
) error {
results := f.MethodCall(f, "AddCharmWithAuthorization", url, channel, macaroon)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) CharmInfo(url string) (*charms.CharmInfo, error) {
results := f.MethodCall(f, "CharmInfo", url)
return results[0].(*charms.CharmInfo), typeAssertError(results[1])
return results[0].(*charms.CharmInfo), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) Deploy(args application.DeployArgs) error {
results := f.MethodCall(f, "Deploy", args)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) GetBundle(url *charm.URL) (charm.Bundle, error) {
results := f.MethodCall(f, "GetBundle", url)
return results[0].(charm.Bundle), typeAssertError(results[1])
return results[0].(charm.Bundle), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) Status(patterns []string) (*params.FullStatus, error) {
results := f.MethodCall(f, "Status", patterns)
return results[0].(*params.FullStatus), typeAssertError(results[1])
return results[0].(*params.FullStatus), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) WatchAll() (*api.AllWatcher, error) {
results := f.MethodCall(f, "WatchAll")
return results[0].(*api.AllWatcher), typeAssertError(results[1])
return results[0].(*api.AllWatcher), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) AddRelation(endpoints ...string) (*params.AddRelationResults, error) {
results := f.MethodCall(f, "AddRelation", variadicStringToInterface(endpoints...)...)
return results[0].(*params.AddRelationResults), typeAssertError(results[1])
return results[0].(*params.AddRelationResults), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) AddUnits(application string, numUnits int, placement []*instance.Placement) ([]string, error) {
results := f.MethodCall(f, "AddUnits", application, numUnits, placement)
return results[0].([]string), typeAssertError(results[1])
return results[0].([]string), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) Expose(application string) error {
results := f.MethodCall(f, "Expose", application)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) SetAnnotation(annotations map[string]map[string]string) ([]params.ErrorResult, error) {
results := f.MethodCall(f, "SetAnnotation", annotations)
return results[0].([]params.ErrorResult), typeAssertError(results[1])
return results[0].([]params.ErrorResult), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) GetCharmURL(serviceName string) (*charm.URL, error) {
results := f.MethodCall(f, "GetCharmURL", serviceName)
return results[0].(*charm.URL), typeAssertError(results[1])
return results[0].(*charm.URL), jujutesting.TypeAssertError(results[1])
}

func (f *fakeDeployAPI) SetCharm(cfg application.SetCharmConfig) error {
results := f.MethodCall(f, "SetCharm", cfg)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) Update(args params.ApplicationUpdate) error {
results := f.MethodCall(f, "Update", args)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) SetConstraints(application string, constraints constraints.Value) error {
results := f.MethodCall(f, "SetConstraints", application, constraints)
return typeAssertError(results[0])
return jujutesting.TypeAssertError(results[0])
}

func (f *fakeDeployAPI) AddMachines(machineParams []params.AddMachineParams) ([]params.AddMachinesResult, error) {
results := f.MethodCall(f, "AddMachines", machineParams)
return results[0].([]params.AddMachinesResult), typeAssertError(results[0])
return results[0].([]params.AddMachinesResult), jujutesting.TypeAssertError(results[0])
}

type fakeBundle struct {
charm.Bundle
*callMocker
*jujutesting.CallMocker
}

func (f *fakeBundle) Data() *charm.BundleData {
results := f.MethodCall(f, "Data")
return results[0].(*charm.BundleData)
}

func NewCallMocker() *callMocker {
return &callMocker{
logger: logger,
results: make(map[string][]*callMockReturner),
}
}

type callMocker struct {
jujutesting.Stub

logger loggo.Logger
results map[string][]*callMockReturner
}

func (m *callMocker) MethodCall(receiver interface{}, fnName string, args ...interface{}) []interface{} {
m.Stub.MethodCall(receiver, fnName, args...)
m.logger.Debugf("Call: %s(%v)", fnName, args)
results := m.Results(fnName, args...)
m.logger.Debugf("Results: %v", results)
return results
}

func (m *callMocker) Results(fnName string, args ...interface{}) []interface{} {
for _, r := range m.results[fnName] {
if reflect.DeepEqual(r.args, args) == false {
continue
}
r.LogCall()
return r.retVals
}
return nil
}

func (m *callMocker) Call(fnName string, args ...interface{}) *callMockReturner {
returner := &callMockReturner{args: args}
// Push on the front to hide old results.
m.results[fnName] = append([]*callMockReturner{returner}, m.results[fnName]...)
return returner
}

type callMockReturner struct {
// args holds a reference to the arguments for which the retVals
// are valid.
args []interface{}

// retVals holds a reference to the values that should be returned
// when the values held by args are seen.
retVals []interface{}

// timesInvoked records the number of times this return has been
// reached.
timesInvoked struct {
sync.Mutex

value int
}
}

func (m *callMockReturner) Returns(retVals ...interface{}) func() int {
m.retVals = retVals
return m.numTimesInvoked
}

func (m *callMockReturner) LogCall() {
m.timesInvoked.Lock()
defer m.timesInvoked.Unlock()
m.timesInvoked.value++
}

func (m *callMockReturner) numTimesInvoked() int {
m.timesInvoked.Lock()
defer m.timesInvoked.Unlock()
return m.timesInvoked.value
}

func typeAssertError(err interface{}) error {
if err == nil {
return nil
}
return err.(error)
}

func variadicStringToInterface(args ...string) []interface{} {
interfaceArgs := make([]interface{}, len(args))
for i, a := range args {
Expand All @@ -1641,7 +1557,8 @@ func variadicStringToInterface(args ...string) []interface{} {
}

func vanillaFakeModelAPI(cfgAttrs map[string]interface{}) *fakeDeployAPI {
fakeAPI := &fakeDeployAPI{callMocker: NewCallMocker()}
var logger loggo.Logger
fakeAPI := &fakeDeployAPI{CallMocker: jujutesting.NewCallMocker(logger)}

fakeAPI.Call("Close").Returns(error(nil))
fakeAPI.Call("ModelGet").Returns(cfgAttrs, error(nil))
Expand Down
42 changes: 27 additions & 15 deletions cmd/juju/cloud/add.go
View file
Open in desktop
Expand Up @@ -20,6 +20,14 @@ import (
"github.com/juju/juju/environs"
)

type CloudMetadataStore interface {
ParseCloudMetadataFile(path string) (map[string]cloud.Cloud, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why mock out the parsing functions? They shouldn't need to change for tests, right? At best, if you want to avoid writing to disk during tests, make it take an io.Reader or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember I used the phrase, "jedi mind trick", in one of our conversations about IoC? Here is a great example!

What we're really writing here is a command, not the parsing logic or anything to do with clouds.yaml. It's purely about interfacing with the user and then deferring logic to other components. Taking in an io.Reader is too much detail for a command; it should deal in concepts, e.g. "go parse this path" or "what do you know about this cloud?" If you defer that kind of logic to another module, you can implement/test that logic in one spot and use it all over the place. It is a very subtle (at least I think so) but important difference.

This is also why in the tests were placing files onto the file system and then reading them back out and checking against strings. Because we are not decoupled from handling cloud files, because we're not just a dumb command, this is the only way we can test.

And although testing is the first thing everyone brings up -- and it is important and what we're discussing -- it's not the main reason to do it this way. The main reason is so that the how is decoupled from the what. And you can change the how without having to also change this code. Rick and I just ran into this situation yesterday.

Copy link
Contributor

Choose a reason for hiding this comment

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

But we actually do need to test that the interactive add cloud code actually produces valid yaml that can be parsed. If we mock that out, then we don't know. I could go change what the parsing code expected, and these tests would all pass, but the code would be wrong and would not work in production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From a product standpoint, yes, we need to know that it is doing the correct thing. This is the responsibility of functional and/or CI tests. The responsibility of these tests is only to know that it's gluing the invocation of commands together with the CloudMetadataStore code correctly.

We can be reasonably sure that it's doing the correct thing because of the emergent correctness between these tests and these tests.

This is why we pass in a well-tested, orthogonal module: so that this code, and these tests, don't have to re-test the same functionality. Functional tests will eclipse both of these packages, and this is why you have fewer functional tests (see this).

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't have to be a functional test somewhere else. It can be a unit test right here. One of the main points of testing this code is to test that it produces valid yaml to pass to Parse. Just merely testing that it calls Parse is not good enough. Putting it into a functional test somewhere else makes failures harder to debug and more likely to be caught much later in the process, which wastes developers time. It also means that someone could easily change one of those tests in a far away place, and we'd never know we lost code coverage until something breaks... hopefully in CI, but possibly in production.

None of the tests in gh/j/j/cloud/cloud_test.go test that add cloud interactive produces valid yaml for cloud.Parse*, AFAICT.

Given that it's trivial to test here, there seems to be no reason not to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we had the fake call functions from within the cloud/ package, the boundaries of our tests are then different from that of our code; i.e. we'd be testing something that may not actually be happening. Not only is checking that it's calling Parse good enough, it's the only logically correct thing to do here. Likewise, I would not expect the cloud/ tests to test an interactive add-cloud run; only that when passed data x into it's functions, it returns data y. The correctness of the add-cloud command when utilized with the cloud/ package is both an emergent property of both test suites, and an explicit property of a functional test.

Functional tests are about testing that all layers are put together properly and actually work. This is the appropriate place to test that an interactive add-cloud works as intended.

Aside from the logical correctness, there are practical implications as to why you don't want to do functional/full-stack tests here. They are arguably no more difficult to write, but they are significantly more difficult to maintain. Again, please see that link about the testing pyramid. There are fewer functional/CI tests for a reason. Our current tests offer further evidence of this.

ParseOneCloud(data []byte) (cloud.Cloud, error)
PublicCloudMetadata(searchPaths ...string) (result map[string]cloud.Cloud, fallbackUsed bool, _ error)
PersonalCloudMetadata() (map[string]cloud.Cloud, error)
WritePersonalCloudMetadata(cloudsMap map[string]cloud.Cloud) error
}

var usageAddCloudSummary = `
Adds a user-defined cloud to Juju from among known cloud types.`[1:]

Expand Down Expand Up @@ -56,11 +64,15 @@ type addCloudCommand struct {

// CloudFile is the name of the cloud YAML file.
CloudFile string

cloudMetadataStore CloudMetadataStore
}

// NewAddCloudCommand returns a command to add cloud information.
func NewAddCloudCommand() cmd.Command {
return &addCloudCommand{}
func NewAddCloudCommand(cloudMetadataStore CloudMetadataStore) cmd.Command {
return &addCloudCommand{
cloudMetadataStore: cloudMetadataStore,
}
}

// Info returns help information about the command.
Expand Down Expand Up @@ -99,7 +111,7 @@ func (c *addCloudCommand) Run(ctxt *cmd.Context) error {
if c.CloudFile == "" {
return c.runInteractive(ctxt)
}
specifiedClouds, err := cloud.ParseCloudMetadataFile(c.CloudFile)
specifiedClouds, err := c.cloudMetadataStore.ParseCloudMetadataFile(c.CloudFile)
if err != nil {
return err
}
Expand All @@ -115,7 +127,7 @@ func (c *addCloudCommand) Run(ctxt *cmd.Context) error {
return errors.Trace(err)
}

return addCloud(c.Cloud, newCloud)
return addCloud(c.cloudMetadataStore, c.Cloud, newCloud)
}

func (c *addCloudCommand) runInteractive(ctxt *cmd.Context) error {
Expand All @@ -127,7 +139,7 @@ func (c *addCloudCommand) runInteractive(ctxt *cmd.Context) error {
return errors.Trace(err)
}

name, err := queryName(pollster)
name, err := queryName(c.cloudMetadataStore, pollster)
if err != nil {
return errors.Trace(err)
}
Expand All @@ -145,12 +157,12 @@ func (c *addCloudCommand) runInteractive(ctxt *cmd.Context) error {
if err != nil {
return errors.Trace(err)
}
newCloud, err := cloud.ParseOneCloud(b)
newCloud, err := c.cloudMetadataStore.ParseOneCloud(b)
if err != nil {
return errors.Trace(err)
}
newCloud.Type = cloudType
if err := addCloud(name, newCloud); err != nil {
if err := addCloud(c.cloudMetadataStore, name, newCloud); err != nil {
return errors.Trace(err)
}
ctxt.Infof("Cloud %q successfully added", name)
Expand All @@ -159,12 +171,12 @@ func (c *addCloudCommand) runInteractive(ctxt *cmd.Context) error {
return nil
}

func queryName(pollster *interact.Pollster) (string, error) {
public, _, err := cloud.PublicCloudMetadata()
func queryName(cloudMetadataStore CloudMetadataStore, pollster *interact.Pollster) (string, error) {
public, _, err := cloudMetadataStore.PublicCloudMetadata()
if err != nil {
return "", err
}
personal, err := cloud.PersonalCloudMetadata()
personal, err := cloudMetadataStore.PersonalCloudMetadata()
if err != nil {
return "", err
}
Expand Down Expand Up @@ -250,11 +262,11 @@ func (c *addCloudCommand) verifyName(name string) error {
if c.Replace {
return nil
}
public, _, err := cloud.PublicCloudMetadata()
public, _, err := c.cloudMetadataStore.PublicCloudMetadata()
if err != nil {
return err
}
personal, err := cloud.PersonalCloudMetadata()
personal, err := c.cloudMetadataStore.PersonalCloudMetadata()
if err != nil {
return err
}
Expand All @@ -279,14 +291,14 @@ func nameExists(name string, public map[string]cloud.Cloud) string {
return ""
}

func addCloud(name string, newCloud cloud.Cloud) error {
personalClouds, err := cloud.PersonalCloudMetadata()
func addCloud(cloudMetadataStore CloudMetadataStore, name string, newCloud cloud.Cloud) error {
personalClouds, err := cloudMetadataStore.PersonalCloudMetadata()
if err != nil {
return err
}
if personalClouds == nil {
personalClouds = make(map[string]cloud.Cloud)
}
personalClouds[name] = newCloud
return cloud.WritePersonalCloudMetadata(personalClouds)
return cloudMetadataStore.WritePersonalCloudMetadata(personalClouds)
}