Add test to ensure docs examples are valid cloud-init configs#355
Conversation
OddBloke
left a comment
OddBloke
left a comment
There was a problem hiding this comment.
Thanks for the PR, James! I think this test is a good candidate for parameterization, and I'll follow up with you on IRC about that. Other than that, I have some inline comments.
| ] | ||
|
|
||
| examples_dir = Path( | ||
| __file__).parent.parent.parent.parent / 'doc' / 'examples' |
There was a problem hiding this comment.
This file (or test) could move around, which would break this determination. Adding an import cloudinit and using cloudinit.__file__ (which is cloudinit/__init__.py, to save you looking it up too) would (a) be more robust, and (b) require less .parenting.
| 'cloud-config-archive.txt', | ||
| ] | ||
|
|
||
| examples_dir = Path( |
There was a problem hiding this comment.
Nice use of pathlib! It's so nice to not have to support Python 2 any longer.
|
|
||
| all_text_files = [f for f in examples_dir.glob('cloud-config*.txt') | ||
| if f.name not in ignored_files] | ||
| for text_file_path in all_text_files: |
There was a problem hiding this comment.
When I see a for loop like this, containing assertions or code under test that could fail, it suggests to me that we have a good candidate for a parametrized test. Could we convert this?
| validate_cloudconfig_file(str(text_file_path), get_schema()) | ||
| except: # noqa | ||
| print('Failed on {}'.format(text_file_path)) | ||
| raise |
There was a problem hiding this comment.
This will only show us the first failure that occurs, without performing tests across all the files. (Parametrizing this test will address this issue.)
| assert examples_dir.is_dir() | ||
|
|
||
| all_text_files = [f for f in examples_dir.glob('cloud-config*.txt') | ||
| if f.name not in ignored_files] |
There was a problem hiding this comment.
cloud-config-archive is a prefix for these files we want to exclude, in the same way that cloud-config is a prefix for the files that we do want to include. Can we drop the static list above if we make this something along the lines of not f.name.startswith('cloud-config-archive') instead?
TheRealFalcon
force-pushed
the
test-examples-schema
branch
from
035d8b4 to
3eeb668
Compare
OddBloke
left a comment
OddBloke
left a comment
There was a problem hiding this comment.
Thanks, this looks good! I have a couple of minor comments inline.
|
|
||
|
|
||
| class TestSchemaDocExamples: | ||
| schema = get_schema() |
There was a problem hiding this comment.
This is a nit (so feel free to not address it), but TestCloudConfigExamples has SCHEMA in all-caps and referred to as TestCloudConfigExamples.SCHEMA in its test: it would be nice to be consistent.
There was a problem hiding this comment.
Would it be ok if I updated the other class? Class variables usually don't use all caps unless they're constants, and self is a less verbose way of accessing a class variable than typing out the whole class every time.
There was a problem hiding this comment.
Would it be ok if I updated the other class?
In general, I expect us all to have ownership of all parts of the codebase, so the answer is yes. In this specific case, the code in question was added by @lucasmoura this week, so lets give him an opportunity to weigh in on what his thinking was.
Class variables usually don't use all caps unless they're constants
This is a constant, in the sense that we only expect to read from it, but I don't feel strongly about this either way.
selfis a less verbose way of accessing a class variable than typing out the whole class every time.
Yep, I'd also be fine with this. I can see using the class name makes sense if we're really treating classes purely as namespaces (in which use self shouldn't have any special meaning), but I agree that it is an unusual spelling (and also makes renaming this class more work, for example).
There was a problem hiding this comment.
Sorry for the long delay, but I don't have any problem with the proposed change. Originally I thought about the schema as a constant, but I do agree that using self for that particular use case is less verbose
Also update all examples to include the cloud-config header if they don't have it LP: #1876414
TheRealFalcon
force-pushed
the
test-examples-schema
branch
from
6ef6f7f to
a3d0167
Compare
|
Addressed most recent comments and pushed a rebase |
3 participants
See https://bugs.launchpad.net/cloud-init/+bug/1876414