Implement a test covering operation of the cat functionality file. #128
Conversation
albu-diku
force-pushed
the
test/functionality-cat-coverage
branch
2 times, most recently
from
7581351 to
cfb7d17
Compare
jonasbardino
changed the title
Make use of the seam that exists by virtue of the functionality files return output objects which are interpreted by a wrapper to unit test the behaviour of cat. Specifically, we can arrange for a suitable environment plus arguments and then check the right output_objects are created. Allow doing this by exposing a variant of the cat main method which returns them rather than invoking the outer wrapper. Name this _main and call it from the original main. Note that this change is in effect a blueprint that can be used when adding coverage for other functionaity files.
albu-diku
force-pushed
the
test/functionality-cat-coverage
branch
from
cfb7d17 to
ac3295d
Compare
|
|
||
| from tests.support import MIG_BASE, MigTestCase, testmain, \ | ||
| fixturefile, fixturefile_normname, \ | ||
| _ensuredirs, _temppath |
There was a problem hiding this comment.
Minor: private and semi-private functions like these (one or two leading underscores) should generally not be used outside the module itself.
While it's not a blocker they should really either be made fully public (i.e. strip leading underscore and make sure they are generally usable) or the import replaced by existing public functions. The shared.fileio already has the makedirs_rec function, which appears to overlap some 90% percent with _ensuredirs.
I'll take a look and either adjust during merge or add the follow-up marker here.
| from mig.shared.functionality.cat import _main as main | ||
|
|
||
|
|
||
| def create_http_environ(configuration, wsgi_variables={}): |
There was a problem hiding this comment.
Minor: as mentioned elsewhere I'd like the WSGI and CGI references in this unit test module to go. From a previous response I understand the origin of this function and that's fine, but delivery method is and should be irrelevant here as we want to (and do) test the underlying functionality backend in general.
I'll probably just generalize the variable name or the function to not even have the wsgi_variables arg during merge.
| return environ | ||
|
|
||
|
|
||
| class MigCgibinCat(MigTestCase): |
There was a problem hiding this comment.
Minor: another CGI leftover to strip.
I'll rename to fit unit test module name during merge and add the missing docstring while at it.
| return _temppath(tmp_path, test_case, ensure_dir=ensure_dir, skip_clean=skip_clean) | ||
|
|
||
|
|
||
| def _temppath(tmp_path, test_case, ensure_dir=False, skip_clean=False): |
There was a problem hiding this comment.
Sorry if this comes out blunt after spending a significant amount of time figuring this code bit out, but on a closer look I don't like this temppath + _temppath construct and the out-of-module use of the latter at all. Probably also because I don't really see the point of exposing this part of the existing temppath as a new semi-private function with a very similar name and then using it directly in test modules.
If the TEST_OUTPUT_DIR anchoring should be skipped in some use cases there, that could easily be achieved with, say, a skip_output_anchor argument instead. That would keep all external use to just temppath and avoid the naming and functionality confusion as well.
I'll see if I can figure that out during merge now that I'm already halfway through.
|
|
||
|
|
||
| def fixturefile_normname(relative_path, prefix=None): | ||
| normname, _ = relative_path.split('--') |
There was a problem hiding this comment.
From experience I know that splitting strings and silently assuming a fixed number of resulting parts like this is a bit fragile and tends to break sooner or later. I'd suggest adding a maxsplit=1 arg to split or use normname=blabla[0] if additional parts are _always_ irrelevant. Otherwise explicitly assert exactly one occurrence of '--'` or split and check part count before assignment.
|
|
||
| def fixturefile_normname(relative_path, prefix=None): | ||
| normname, _ = relative_path.split('--') | ||
| return os.path.join(prefix, normname) if prefix else normname |
There was a problem hiding this comment.
On a pedantic note I'm generally not too fond of these one-liner conditionals as they tend to save little and make code less readable IMO. E.g in this case it could easily hide details like if you really meant to test prefix to be logically True or actually wanted to only target the default value with is not None. Here prefix='' would not really make a difference, but for the general sake it might.
|
Merged through svn with the comments addressed during merge. The only one left alone was the string split and implicit assumption about two resulting parts. |
Make use of the seam that exists by virtue of the functionality files
return output objects which are processed by a wrapper to unit test
the behaviour of cat.
Specifically, we can arrange for a suitable environment plus arguments
and then check the right output_objects are created. Allow doing this
by exposing a variant of the cat main method which returns them rather
than invoking the outer wrapper. Name this _main and call it from the
original main.
Note that this change is in effect a blueprint that can be used when
adding coverage for other functionality files.