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

Status machine filter tests. #8724

Merged
merged 3 commits into from May 18, 2018

Conversation

anastasiamac
Copy link
Contributor

Description of change

There are outdated bugs around status and its ability to filter by machine id.

This PR proposes tests to ensure that the functionality was delivered, is functioning as expected and that we do not regress in the future.

These tests ensure that:

  • we can filter by machine id to see units, and hence, applications that are relevant to this machine;
  • we filter on machine id exactly, i.e. searching for machine "1" does not also return anything related to machine "10".

Bug reference

https://bugs.launchpad.net/juju/+bug/1748915
https://bugs.launchpad.net/juju/+bug/1584170

@anastasiamac
Copy link
Contributor Author

Copy link
Member

@wallyworld wallyworld left a comment

Choose a reason for hiding this comment

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

Thanks! More tests yay.

func (s *StatusSuite) SetUpTest(c *gc.C) {
s.JujuConnSuite.SetUpTest(c)

func (s *StatusSuite) assertMultipleRelationsBetweenApplications(c *gc.C) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe setupMultipleRelationsBetweenApplications
assertFoo() is typically used to test the results of doing something after it has happened and we want to check that the code behaved as expected.
Here, we are setting up a scenario for which we then want to run a test

@@ -68,6 +67,7 @@ func (s *StatusSuite) run(c *gc.C, args ...string) *cmd.Context {
}

func (s *StatusSuite) TestMultipleRelationsInYamlFormat(c *gc.C) {
s.assertMultipleRelationsBetweenApplications(c)
Copy link
Member

Choose a reason for hiding this comment

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

As per above comment, this would be clearer as s.setupMultipleRelations.....

context := s.run(c, "status", "--relations")
c.Assert(cmdtesting.Stdout(context), jc.Contains, `
Relation provider Requirer Interface Type Message
wordpress:juju-info logging:info juju-info subordinate joining
wordpress:logging-dir logging:logging-directory logging subordinate joining
`[1:])
}

func (s *StatusSuite) assertServeralUnitsOnAMachines(c *gc.C) {
Copy link
Member

Choose a reason for hiding this comment

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

here too etc

func (s *StatusSuite) TestStatusWhenFilteringByMachine(c *gc.C) {
s.assertServeralUnitsOnAMachines(c)

// Put a unit from the application C on a different machine.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know what application C was until a dig in a bit. Maybe reword the comment just to say we are adding a new application to a machine ti ensure it is filtered out as expected.

Application: application,
Machine: machine10,
})
context := s.run(c, "status", "1")
Copy link
Member

Choose a reason for hiding this comment

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

Just to ensure and check there really is a machine 10, can we also run an check that "status 10" works as expected. Otherwise the test could pass by mistake.

@anastasiamac
Copy link
Contributor Author

$$merge$$

@jujubot jujubot merged commit 80f1d20 into juju:2.3 May 18, 2018
@anastasiamac anastasiamac deleted the status-machine-filter-tests-23 branch May 18, 2018 05:10
jujubot added a commit that referenced this pull request May 18, 2018
@jameinel jameinel mentioned this pull request May 21, 2018
jujubot added a commit that referenced this pull request May 21, 2018
#8737

## Description of change

Merge 2.3 into develop, including the Mongo changes and resolving any small issue with the double ported status changes.

This includes these new patches:

 prdesc Merge pull request #8718 from jameinel/2.3-mongodb-version

And these patches that were independently directly merged into develop:

 prdesc Merge pull request #8727 from anastasiamac/empty-status-friendly-msg
 prdesc Merge pull request #8724 from anastasiamac/status-machine-filter-tests-23
 prdesc Merge pull request #8717 from anastasiamac/multi-relations-status-test


## QA steps

See individual patches.

## Documentation changes

See individual changes

## Bug reference

None.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants