Adding unit tests, golden files, and supplemental labels #10
Conversation
|
This is now in a state to be actually looked at. The changes also now include new functionality in the current program to do the following:
Let me know if you have any questions. |
|
I have added the new command line argument function, now named Fixes #8 |
There was a problem hiding this comment.
- I haven't tried to run the code, so these comments are just from visual inspection.
- I know it's a pain, but give some consideration to what I suggested earlier about creating the golden files by hand before running the tests. Right now I think the golden files are being generated by the code, and there are cases where they are wrong. If you had written them by hand first, you would have written the correct answer and then found a bug when running the test. It takes a lot longer to do than just auto-generating them and looking at them to see if you think they're correct, but you will catch more bugs this way.
- All files related to testing should go under the single
test_filesdirectory. There shouldn't be a separate top-levelsamplesdirectory. Then you can create sub-directories under there for different purposes. For example,test_files/sample_inputandtest_files/expected. Then you don't need to add_successto the end of your filenames, although it doesn't hurt to leave it there. - Your input files seem to be split between two locations, e.g.
test_files/tester_label_1.xmlandsamples/sample_elements.txt. You could put all these undertest_files/sample_inputor divide them further liketest_files/sample_input/element_filesandtest_files/sample_input/labels. - The clean headers option needs to do something with
<and>since those are also illegal identifiers in strings. I think it would be easiest just to replace<1>with_1.
tests/test_pds4_create_xml_index.py
Outdated
| ) | ||
|
|
||
| def test_sort_by(golden_file, new_file, cmd_line): | ||
| # Create a temporary directory in the same location as the test_files directory |
There was a problem hiding this comment.
Why do you want it in the same location as the test_files directory? Just let the OS put it where it normally puts temp files.
| ) | ||
| ] | ||
| ) | ||
|
|
There was a problem hiding this comment.
I would not have a blank line between the @pytest.mark.parametrize and the function definition. That way it's clear they are bound together as one entity.
tests/test_pds4_create_xml_index.py
Outdated
|
|
||
|
|
||
|
|
||
| # Set parameters values that you would like to pass into test_sort_by. |
There was a problem hiding this comment.
Rather than explaining how @pytest.mark.parametrize works, which people should already know if they're looking at a pytest file, I think it would be more useful to explain what you're actually testing in each section.
tests/test_pds4_create_xml_index.py
Outdated
|
|
||
| # These two variables are the same for all tests, so we can either declare them as | ||
| # global variables, or get the root_dir at the setup stage before running each test | ||
| root_dir = Path(__file__).resolve().parent.parent |
There was a problem hiding this comment.
By convention, global variables are usually written in all-caps.
tests/test_pds4_create_xml_index.py
Outdated
| assert config_object['pds:ASCII_Short_String_Collapsed']['anticipated'] == 'anticipated_alt' | ||
|
|
||
|
|
||
| # now, a bad config file |
| i += 1 | ||
| label_results[key_new] = label_results.pop(key) | ||
|
|
||
|
|
| '{'+namespace+'}', prefixes[namespace]+':') | ||
| xml_results[key_new] = xml_results[key] | ||
| del xml_results[key] | ||
| elements = () |
There was a problem hiding this comment.
This is a real nit, but since tuples are immutable they are not meant to be appended to. If you append an object to a tuple with +=, it creates a whole new tuple from scratch, copying over all the old data. However, lists are mutable and are meant to be extended. So x.append(y) is an efficient operation, while x += (y,) (where x is a tuple) is less efficient.
| for part in parts: | ||
| if '<' in part: | ||
| part = part.split('<') | ||
| elements += (part[0],) |
There was a problem hiding this comment.
Do you really only want to include elements that have <n> at the end?
There was a problem hiding this comment.
At this point in the code, all elements within every XPath header should have <n> at the end.
| for item in elements: | ||
| file.write("%s\n" % item) | ||
| else: | ||
|
|
| negated.append(pat) | ||
| else: | ||
| positive.append(pat) | ||
| for key, value in input_dict.items(): |
There was a problem hiding this comment.
One thing that might be useful here is allowing the "verbose" option to give details about what is happening here. For example, you could print:
Adding elements due to: **/*a*
/pds:/whatever
/pds:/whatever
Deleting elements due to: !**/*ab*
/whatever
This pull request will introduce the unit tests for the indexing tools.
Fixes #7
Fixes #6
Fixes #5