Refactor resources tests - integrate versions and improve test coverage #297

hayfield · 2018-03-15T09:03:22Z

Make sweeping and wide-ranging improvements to tests of the resources module.

The two key purposes of these changes are:

Ensure resources functions handle versions as expected
Significantly improve direct test coverage (rather than the indirect testing that occurred previously)

…-arg

This is required for the completion of some things in #255

A fixture is removed following a mis-remembering of how 'version-independent' works.

The name was longer than needed following previous iterations of the name

This includes: - decimal - integer - version-independent

This makes it clearer that the class works with creating paths, rather than XPaths (which are something completely different...)

…path

This brings it in line with #255

…ersion-handling'

This is the minimal amount of change required to make the tests pass. This should very much not be deemed a final implementation - there is a lot of messy type checking. Refactoring of code in this commit should definitely be undertaken.

…on-handling'

…ion-handling'

…on-handling

…n-handling

Strings consisting of whitespace and numbers were being decimalised. This is not desired since the value does not represent an integer. This improves the testing so that these values are not decimalised.

…on-handling'

Oops...

This is a process starting with the lowest level components, and building up to the larger components. Since the resources module was originally created, it has been redesigned multiple times. Across this period, the tests have not been rearchitectured. This process will turn the resource tests into a state that works for the current module state, rather than that from a year ago.

This provides better bounds on what permitted values are, in turn enabling some simplification of the matching algorithm. The name of the version_independent folder has been changed so that it matches the regex that has been set for resource folder names.

This tests valid and invalid inputs, while also adding an extra check over the previous implementation.

The includes both valid and invalid paths and versions. A couple of changes and tests have needed adding to improve correctness of functions that return components of this path.

The general functionality is as-per Rulesets and Schemas. The one difference is around major versions, where they remain as majors rather than being converted to minors.

…ctions The version decorators require that the decorated function has a version argument. When the input function takes no parameters, an IndexError was being raised. This makes it more appropraite, a TypeError.

The improving of wrapper names did not account for this change

This adds more thorough testing for obtaining lists of Codelist paths. The current situation doesn't allow differentiation between types of Codelist - at the moment Embedded and Non-Embedded. This should be investigated fully once a decision in this area has been made.

Now that the entire file has been refactored, the marks that differentiate new and old tests can be removed.

dalepotter · 2018-03-20T14:41:48Z

iati/resources.py

    """Find the paths for all Codelists at the specified version of the Standard.

    Args:
-        version (str): The version of the Standard to return the Codelists for. Defaults to iati.version.STANDARD_VERSION_ANY.
+        version (str / int / Decimal / iati.Version): The version of the Standard to return the Codelists for.


I think we need to be clear that this returns paths to embedded and non-embedded codelists. I had to run it to be sure!

dalepotter

Adding comments as a I go...

dalepotter

More comments

dalepotter · 2018-03-20T14:48:36Z

iati/resources.py

+        version (str / int / Decimal / iati.Version): The version of the Standard to return the Codelists for.
+            Decimal: Return paths for the specified version of the Standard.
+            Integer: Return paths for the latest Decimal version within the given integer.
+            Version-independent: Return paths for Non-Embedded Codelists.


wrt to version independent info, is this expected?

>>> iati.resources.get_codelist_paths(iati.version.STANDARD_VERSION_ANY) []

That is... not the documented behaviour!

There are a couple of issues around the naming of the folder containing NE Codelists (being codelists_non_embedded/ rather than version_independent/codelists/) and the default module currently expecting an empty array.

I'd suggest that management and handling of NE Codelists should be looked at as standalone Issue (with this particular docstring being updated to reflect the current behaviour).

...the problem with default is due to STANDARD_VERSION_ANY being added as a thing after it was tested, so there not being any tests against this value (so it's deem to not be a permitted value at all!)

dalepotter · 2018-03-20T14:49:38Z

iati/resources.py


    Returns:
        list(str): A list of paths to all of the Codelists at the specified version of the Standard.

    Todo:
-        Further exploration needs to be undertaken in how to handle pre-1.04 versions of the Standard.
+        Look to provide an argument that allows the returned list to be restricted to only Embedded or only Non-Embedded Codelists.


parameter that is embedded=True or non_embedded=True???

Adding functionality along these lines is out of scope for this PR so will need to be designed and undertaken separately.

It is desired that there is a way to return only NE-Codelist paths. This is yet to be implemented and should be undertaken as a slightly wider task of looking into keeping track of how different Codelists are managed.

hayfield · 2018-03-20T16:16:33Z

#308 turns the TODOs about improving the distinction between Embedded and Non-Embedded Codelists (as noted in review comments) into an Issue.

A PR now exists, so the number can be added

The previous names were inconsistent with others in the same class, and did not match the name of the function they were testing. The new names are consistent and match.

A number of functions in the resources module still had default values for the version argument. Explicit is better than implicit. As such, the default values have been removed and tests added to ensure TypeErrors are raised as appropriate. It was documented in the changelog that this was already the case, though the removal of defaults had only been undertaken for some functions.

37cca6d updated code without updating the docstrings. This updates the docstrings.

There are tests checking that there is no default value. As such, there being no parameter shouldn't be a linting error.

dalepotter · 2018-03-21T14:53:12Z

iati/resources.py

-        return 'version-independent'
-    elif isinstance(version, str):
+        return PATH_VERSION_INDEPENDENT
+    elif isinstance(version, (str, int)) and not isinstance(version, bool):


Suggest add todo docstring to note that this logic is necessary given lack of functionality such as Version.is_major.

hayfield · 2018-03-21T15:51:55Z

iati/version.py

@@ -355,7 +364,12 @@ def decimalise_integer(input_func):
    """
    def wrap_decimalise_integer(*args, **kwargs):
        """Act as a wrapper to convert input Integer Version numbers to a normalised format Decimal Version."""
-        version = _decimalise_integer(args[0])
+        try:


Extract to version = _extract_argument(args, arg_idx=0)

A number of small changes made while reviewing the resources code from top to bottom.

Change as requested in: #297 (review)

Some code was repeated many times. That was bad. It is now in a function. This is less bad.

There were previously no tests for this. There are now tests for this.

dalepotter · 2018-03-21T15:09:31Z

iati/tests/test_resources.py

-    @pytest.mark.parametrize('path_component', [
-        'resources',
-        'standard'
+class TestResourcePathCreationCodelistMapping(object):


Ensure that there are typeerror and junnk value tests.

dalepotter

Comments from the code review addressed, so approving...

hayfield added 30 commits November 28, 2017 09:34

Test that test data for various versions are being detected

8c1004d

Merge branch 'improve-version-parameter' into 218ify-resource-version…

9c7999b

…-arg

Add a fixture for specifying all standard versions, or independent

86e2332

This is required for the completion of some things in #255

Test the creation of Codelist resource paths

9af9dea

A fixture is removed following a mis-remembering of how 'version-independent' works.

Rename a function

351a74d

The name was longer than needed following previous iterations of the name

Add a fixture to actually provide all versions of the IATI Standard

e9a4d71

This includes: - decimal - integer - version-independent

Remove an 'X' from a class name

c3d8be0

This makes it clearer that the class works with creating paths, rather than XPaths (which are something completely different...)

Move a create_x_path function to new class

fb4f9b1

Merge branch 'minor-versions-constant' into 218ify-resource-create-x-…

8b03b82

…path

Test the creation of Codelist Mapping file resource paths

3c72578

This brings it in line with #255

Fuzz the finding of a Codelist Mapping File path

ceef8a5

Test that creation of Codelist paths raises relevant errors

71110b5

Merge branch '218ify-resource-create-x-path' into 'improve-resource-v…

85499b8

…ersion-handling'

Make resources tests pass

7cad2b5

This is the minimal amount of change required to make the tests pass. This should very much not be deemed a final implementation - there is a lot of messy type checking. Refactoring of code in this commit should definitely be undertaken.

Update documentation for permitted version arg values

a4b219b

Merge branch 'improve-version-parameter' into 'improve-resource-versi…

82d3491

…on-handling'

Merge branch 'prevent-decimal-attr-error' into 'improve-resource-vers…

db320ed

…ion-handling'

Merge branch 'prevent-decimal-attr-error' into improve-resource-versi…

bea658c

…on-handling

Merge branch 'improve-version-parameter' into improve-resource-versio…

1cf576b

…n-handling

Improve testing of requesting Codelist Mapping resources

2a28686

Fix decimalisation of invalid major versions

01b403d

Strings consisting of whitespace and numbers were being decimalised. This is not desired since the value does not represent an integer. This improves the testing so that these values are not decimalised.

Merge branch 'improve-version-parameter' into 'improve-resource-versi…

be2965a

…on-handling'

Actually save a file between resolving a merge conflict and committing

a3f8112

Oops...

Merge branch 'improve-version-parameter' into refactor-resources-tests

f12a6c0

Fix a docstring to refer to the correct variable name.

c0292a3

Test resource function folder_path_for_version

032af8e

This tests valid and invalid inputs, while also adding an extra check over the previous implementation.

Test the resource path_for_version function

cfd1c48

The includes both valid and invalid paths and versions. A couple of changes and tests have needed adding to improve correctness of functions that return components of this path.

Merge branch 'improve-version-parameter' into refactor-resources-tests

c1e4e13

hayfield added 5 commits March 15, 2018 11:28

More thoroughly test resources.create_codelist_path()

244203b

The general functionality is as-per Rulesets and Schemas. The one difference is around major versions, where they remain as majors rather than being converted to minors.

standardise -> normalise

9f9f728

The improving of wrapper names did not account for this change

Tidy up some development placeholders

c114412

Now that the entire file has been refactored, the marks that differentiate new and old tests can be removed.

This was referenced Mar 20, 2018

Add permanent 2.03 support #307

Merged

Improve version argument in **/resources.py #255

Closed

dalepotter reviewed Mar 20, 2018

View reviewed changes

Update documentation for iati.resources.get_codelist_paths()

b26c2c6

It is desired that there is a way to return only NE-Codelist paths. This is yet to be implemented and should be undertaken as a slightly wider task of looking into keeping track of how different Codelists are managed.

hayfield added 5 commits March 20, 2018 16:32

Add PR numbers for resource test changelog items

28aece8

A PR now exists, so the number can be added

Make test function names match what they are testing

7ec75b1

The previous names were inconsistent with others in the same class, and did not match the name of the function they were testing. The new names are consistent and match.

Remove now False statements from docstrings

f89e7f8

37cca6d updated code without updating the docstrings. This updates the docstrings.

Add a pylint disable for missing parameter

3782fd2

There are tests checking that there is no default value. As such, there being no parameter shouldn't be a linting error.

hayfield mentioned this pull request Mar 21, 2018

Improve version parameter #223

Merged

15 tasks

dalepotter reviewed Mar 21, 2018

View reviewed changes

hayfield commented Mar 21, 2018

View reviewed changes

hayfield added 4 commits March 21, 2018 16:14

Make review changes

02d1433

A number of small changes made while reviewing the resources code from top to bottom.

Add a clarity comment based on review

c880a6e

Change as requested in: #297 (review)

Extract some common version functionality into a function

d19a6df

Some code was repeated many times. That was bad. It is now in a function. This is less bad.

Test that create_codelist_mapping_path() TypeError-s correctly

2f62bf9

There were previously no tests for this. There are now tests for this.

dalepotter reviewed Mar 21, 2018

View reviewed changes

dalepotter approved these changes Mar 21, 2018

View reviewed changes

hayfield added complete A PR that is in a state that is ready for review. and removed incomplete A PR that is in a state that is not ready for review. labels Mar 22, 2018

hayfield merged commit 5021c1d into improve-version-parameter Mar 22, 2018

hayfield deleted the refactor-resources-tests branch March 22, 2018 10:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor resources tests - integrate versions and improve test coverage #297

Refactor resources tests - integrate versions and improve test coverage #297

hayfield commented Mar 15, 2018

dalepotter Mar 20, 2018 •

edited

Loading

hayfield Mar 20, 2018

dalepotter left a comment

dalepotter left a comment

dalepotter Mar 20, 2018

hayfield Mar 20, 2018

hayfield Mar 20, 2018

hayfield Mar 20, 2018

dalepotter Mar 20, 2018

hayfield Mar 20, 2018

hayfield Mar 20, 2018

hayfield commented Mar 20, 2018 •

edited

Loading

dalepotter Mar 21, 2018

hayfield Mar 21, 2018

hayfield Mar 21, 2018

hayfield Mar 21, 2018

dalepotter Mar 21, 2018

dalepotter left a comment

Refactor resources tests - integrate versions and improve test coverage #297

Refactor resources tests - integrate versions and improve test coverage #297

Conversation

hayfield commented Mar 15, 2018

dalepotter Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalepotter left a comment

Choose a reason for hiding this comment

dalepotter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayfield commented Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalepotter left a comment

Choose a reason for hiding this comment

dalepotter Mar 20, 2018 •

edited

Loading

hayfield commented Mar 20, 2018 •

edited

Loading