Skip to content

extends must be copied from override file#2206

Merged
mnowster merged 1 commit into
docker:masterfrom
dnephin:fix_extends_from_second_file
Oct 20, 2015
Merged

extends must be copied from override file#2206
mnowster merged 1 commit into
docker:masterfrom
dnephin:fix_extends_from_second_file

Conversation

@dnephin
Copy link
Copy Markdown

@dnephin dnephin commented Oct 16, 2015

Fixes #2205

merge_service_dicts() which is used for extends doesn't copy extends, but we need to copy extends for overriding of files.

In the test for this fix I've used a tempdir for creating the necessary file instead of using a real file. I think this makes the test easier to understand because the fixtures are inline, instead of a separate file.

If we're happy with this approach, I think we could convert a few of our other unit tests to use this setup.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's not immediately clear why you're doing this? How does creating a temporary directory fit into the description of what this test is doing around extends? Could this have been achieved with a fixture?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Could this have been achieved with a fixture?

If we go with the py.test way of doing fixtures, maybe. With unittest fixtures, no.

Fixtures are just "common test setup", but there is always other test setup that isn't all that common. This is just setting up the environment so the test exercises the expected functionality.

It's creating a common.yml which is referenced from the config above.

I'm not sure how to make this more clear.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Added more detail to the PR description about why this is different

@mnowster
Copy link
Copy Markdown

I'm LGTM for both code and adopting the new testing approach. I understand now and can see the benefit of having the detail of what you're testing inline with the test.

@mnowster
Copy link
Copy Markdown

@aanand are you cool with the direction of this testing approach?

@aanand
Copy link
Copy Markdown

aanand commented Oct 19, 2015

Yeah, I like it. Fixtures are good in that an actual directory with actual files is nice and easy to understand, but I never liked the coupling-at-a-distance.

@dnephin
Copy link
Copy Markdown
Author

dnephin commented Oct 19, 2015

Ya, I think for end-to-end tests having the files could be good, but for unit tests it's easier to just do it inline. Most of our files exist for our integration tests. I think there are only a couple used by unit tests.

@dnephin dnephin force-pushed the fix_extends_from_second_file branch from 64f95c8 to 938d49c Compare October 19, 2015 17:41
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin
Copy link
Copy Markdown
Author

dnephin commented Oct 19, 2015

Rebased

mnowster added a commit that referenced this pull request Oct 20, 2015
extends must be copied from override file
@mnowster mnowster merged commit 5ed9f9b into docker:master Oct 20, 2015
@dnephin dnephin deleted the fix_extends_from_second_file branch October 20, 2015 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"extends" and multiple yml files don't work together

4 participants