Commit
The argument differs from the others because its type is not string - therefore, argparse performs some manipulations with the supplied file, and these manipulations have to be taken into consideration.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| import os | ||
| import sys | ||
| import tempfile | ||
| import unittest | ||
|
|
||
|
|
||
|
|
@@ -203,8 +204,49 @@ def f(self): | |
| add_override_hdfs_mode(m) | ||
|
|
||
|
|
||
| class ProcessorStageConfigArgTestCase(unittest.TestCase): | ||
| def setUp(self): | ||
| self.stage = pyDKB.dataflow.stage.ProcessorStage() | ||
| self.fake_config = tempfile.NamedTemporaryFile(dir='.') | ||
|
|
||
| def tearDown(self): | ||
| self.stage = None | ||
| if not self.fake_config.closed: | ||
| self.fake_config.close() | ||
| self.fake_config = None | ||
|
|
||
| def test_correct_c(self): | ||
| self.stage.parse_args(['-c', self.fake_config.name]) | ||
| self.assertIsNotNone(getattr(self.stage.ARGS, 'config')) | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong. 
Evildoor
Author
Contributor
|
||
|
|
||
| def test_correct_config(self): | ||
| self.stage.parse_args(['--config', self.fake_config.name]) | ||
| self.assertIsNotNone(getattr(self.stage.ARGS, 'config')) | ||
|
|
||
| def test_missing_c(self): | ||
| self.fake_config.close() | ||
| with self.assertRaises(SystemExit) as e: | ||
| self.stage.parse_args(['-c', self.fake_config.name]) | ||
|
|
||
| def test_missing_config(self): | ||
| self.fake_config.close() | ||
| with self.assertRaises(SystemExit) as e: | ||
| self.stage.parse_args(['--config', self.fake_config.name]) | ||
|
|
||
| def test_unreadable_c(self): | ||
| os.chmod(self.fake_config.name, 0300) | ||
| with self.assertRaises(SystemExit) as e: | ||
| self.stage.parse_args(['-c', self.fake_config.name]) | ||
|
|
||
| def test_unreadable_config(self): | ||
| os.chmod(self.fake_config.name, 0300) | ||
| with self.assertRaises(SystemExit) as e: | ||
| self.stage.parse_args(['--config', self.fake_config.name]) | ||
|
|
||
|
|
||
| test_cases = ( | ||
| ProcessorStageArgsTestCase, | ||
| ProcessorStageConfigArgTestCase, | ||
| ) | ||
|
|
||
|
|
||
|
|
||
5 comments
on commit fdb498a
There was a problem hiding this comment.
Haven`t we discussed the idea of placing this test case into a separate file?
There was a problem hiding this comment.
I believe that we discussed the separate test case, but not the separate file. If I understand correctly, the distribution of tests among test cases is mostly tied to setUp() and tearDown() (and a separate case was made for config argument), while distribution of cases among files is up to us, unless we need to perform some actions before/after working with test cases (in other words, some kind of file-level setUp() and tearDown()). I don't think that this situation warrants a separate file, because:
- File-level actions so far include imports and related things, and the preparation of tests and test cases. I don't see how splitting the file into two will help here.
- Before config-related tests, this file had a single test case. Why make a second case and immediately move it into a new file? It's not that different.
- So far we agreed upon the test files and their names being tied to the code files. Why change this rule?
There was a problem hiding this comment.
Huh, maybe it was about separate test cases indeed; yet I believed that two classes will be placed into separate files, and the answer to your item 2 explains why.
- File-level actions so far include imports and related things, and the preparation of tests and test cases. I don't see how splitting the file into two will help here.
Won`t help at all, right.
- Before config-related tests, this file had a single test case. Why make a second case and immediately move it into a new file? It's not that different.
It is a common practice to place each class into a separate file, and I believe that it had not appeared for nothing ;)
For me, the reason is to improve readability:
- by keeping files as small as possible (yet staying reasonable about the number of files, of course);
- by indicating: "this two classes may be neighbors, yet you don`t need to read through them together; one can easily be understood without another."
I mean, when I see a file with code, I automatically suggest that it contains a set of smaller independent pieces of the code (like a collection of simple functions) or highly interlinked code (so that one can`t throw a piece of it away and still be able to understand what`s going on). And this case is obviously not about the first -- there are two classes (already collections of methods) plus a number of high-level functions; so my first intention is to understand the structure of the file, how are these classes and functions and pieces of the code are linked together. And when I find out that, actually, there is one piece that is not linked to another, I feel screwed ;) Why would anyone make this intentionally?
- So far we agreed upon the test files and their names being tied to the code files. Why change this rule?
Right; but what the idea of this agreement? The idea, for me, is to establish visible correspondence between the main code location and the test code location. But what`s the reason to stick with the file-to-file correspondence? It can be file-to-submodule correspondence, why not. Or file-to-files, maybe; yet I am not sure how it will look.
So, there is a reason to split test cases into separate files (for me, at least); is there a reason to keep them in a single file?
There was a problem hiding this comment.
Can't argue with your logic. Then, if we adopt the idea of splitting the file into two, we have two questions:
-
File names. So far it was decided to make a single test file for a single code file, and name the test file by adding "test_" to the name of a code file. I suggest that we keep it that way if the tests are simple enough, and if there are two or more test files, then additional ones (or all of them) should have additional description in the middle of the name ("test" at the beginning is required), i.e. "test_ProcessorStage.py", "test_config_ProcessorStage.py" and "test_some_obscure_function_ProcessorStage.py". Since the tests are usually invoked by discover, long names shouldn't be a problem.
-
Test cases. If each test case goes into a separate file, then some things can be simplified. One of them is the names of test cases - if each test case has its own file, then it doesn't really need a specific name, because the output of testing looks like this:
test_correct_c (test_ProcessorStage.ProcessorStageConfigArgTestCase) ... ok test_correct_config (test_ProcessorStage.ProcessorStageConfigArgTestCase) ... okWhen we split the files, the info will be moved into the file name, so the descriptive test case name is not needed:
test_correct_c (test_config_ProcessorStage.Case) ... ok test_correct_config (test_config_ProcessorStage.Case) ... okAnother option is to reverse the approach: name the files in a simple way, like "test_1_ProcessorStage.py", and move the rest into the test case name (or even go further and throw "ProcessorStage" into the case name as well). It will prevent the files from having long names, but the tester would have to remember which file holds what in some situations. This includes testing separate files/cases/methods instead of simply invoking discover: the syntax is "python -m unittest file(.case(.method))".
So, unless there are objections or comments, then I'll split the file into two, simplify the case-processing mechanism, set a fixed name for a test case, and change the "how-to:tests" accordingly.
There was a problem hiding this comment.
@Evildoor, my apologies for the late reply.
I like the idea of all TestCase subclasses with simple name Case; having files named test_N.py will be confusing, just as you said.
Not sure if tests/test_config_ProcessorStage.py is better then tests/ProcessorStage/test_configfile.py (it can also be tests/test_ProcessorStage/configfile.py, but that would require some kind of load_tests(), that would add to TestSuite all the tests from all <filename>.Case classes). But let`s go the way you suggested, it`s never too late to change the approach.
Should it be
NotNone, or it can be checked against a specific class?