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

lxd: always remove existing device for project folder #1488

Merged
merged 2 commits into from Aug 22, 2017

Conversation

kalikiana
Copy link
Contributor

As a follow-up on #1483 this change removes an existing device for the project folder.

@kalikiana kalikiana self-assigned this Aug 16, 2017
Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Looks good, the tests are getting harder to follow though. We might want to start trimming down what we expect from the tests and have sane defaults for things like FakeLXD().name and so on to not need to set them and unset them for the odd cases.

@ElOpio mind testing this?

@kalikiana
Copy link
Contributor Author

@sergiusens I think you may have misunderstood how the test works. FakeLXD gets values like name and status from the mock commands that are executed. So there's no need to set anything. However, this particular test fakes an existing state of FakeLXD without any commands. If you prefer, the test could actually execute commands to create a container.

@kyrofa
Copy link
Contributor

kyrofa commented Aug 18, 2017

@kalikiana I think what @sergiusens means is that there are a lot of container tests in here all mocking similar things. Perhaps this would be a good opportunity to refactor these tests to have a common base class that extracts the common mocks, which will increase readability for all of them.

Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

I tested multiple container builds with this branch. They all worked, and the change looks good to me too.

I agree that the tests code be simplified if we skip some assertions. Like the message Network connection established is not relevant for this test, so it could be removed. Or at least, hidden in a helper method.

@sergiusens sergiusens added this to the 2.34 milestone Aug 22, 2017
@sergiusens sergiusens merged commit 11450b4 into canonical:master Aug 22, 2017
kalikiana added a commit to kalikiana/snapcraft that referenced this pull request Sep 21, 2017
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

4 participants