New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract shared fixture from test cases #60

Merged
merged 4 commits into from Dec 5, 2017

Conversation

Projects
None yet
2 participants
@robertknight
Contributor

robertknight commented Dec 4, 2017

Many tests in util_test.py duplicated the same fixture data for fields
that are expected to be present in every ES annotation JSON document.

Extracting the fixture will make it easier to add new fields to it.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Dec 4, 2017

Codecov Report

Merging #60 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   88.23%   88.44%   +0.21%     
==========================================
  Files          10       10              
  Lines         544      554      +10     
==========================================
+ Hits          480      490      +10     
  Misses         64       64
Impacted Files Coverage Δ
tests/bouncer/util_test.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ce1ad...f844ed5. Read the comment docs.

codecov bot commented Dec 4, 2017

Codecov Report

Merging #60 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
+ Coverage   88.23%   88.44%   +0.21%     
==========================================
  Files          10       10              
  Lines         544      554      +10     
==========================================
+ Hits          480      490      +10     
  Misses         64       64
Impacted Files Coverage Δ
tests/bouncer/util_test.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ce1ad...f844ed5. Read the comment docs.

@fatbusinessman

Overall, I like this change, but I’ve got a couple of questions and spotted one thing that might be a mistake.

assert document_uri == "http://example.com/foo.pdf"
def test_parse_document_raises_when_uri_from_web_uri_not_string_for_pdfs():
def test_parse_document_raises_when_uri_from_web_uri_not_string_for_pdfs(es_annotation_doc):

This comment has been minimized.

@fatbusinessman

fatbusinessman Dec 5, 2017

This test case seems to be testing with significantly different input than it did before; is that intentional? If so, why?

Before:

{
    "_id": "annotation_id",
    "_source": {
        "target": [{
            "source": "urn:x-pdf:the-fingerprint",
            "selector": []
        }],
        "group": "__world__",
        "shared": True,
        "document": {"web_uri": 52}
    }
}

After:

{
    "_id": "annotation_id",
    "_source": {
        "target": [{
            "source": 52,
            "selector": [{}]
        }],
        "group": "__world__",
        "shared": True,
    },
    "document": {"web_uri": "http://example.com/foo.pdf"}
}
@fatbusinessman

fatbusinessman Dec 5, 2017

This test case seems to be testing with significantly different input than it did before; is that intentional? If so, why?

Before:

{
    "_id": "annotation_id",
    "_source": {
        "target": [{
            "source": "urn:x-pdf:the-fingerprint",
            "selector": []
        }],
        "group": "__world__",
        "shared": True,
        "document": {"web_uri": 52}
    }
}

After:

{
    "_id": "annotation_id",
    "_source": {
        "target": [{
            "source": 52,
            "selector": [{}]
        }],
        "group": "__world__",
        "shared": True,
    },
    "document": {"web_uri": "http://example.com/foo.pdf"}
}

This comment has been minimized.

@robertknight

robertknight Dec 5, 2017

Contributor

Gah! This was indeed a mistake. The 52 should have been assigned to the "web_uri" field.

@robertknight

robertknight Dec 5, 2017

Contributor

Gah! This was indeed a mistake. The 52 should have been assigned to the "web_uri" field.

Show outdated Hide outdated tests/bouncer/util_test.py Outdated
Show outdated Hide outdated tests/bouncer/util_test.py Outdated

robertknight added some commits Dec 4, 2017

Extract shared fixture from test cases
Many tests in `util_test.py` duplicated the same fixture data for fields
that are expected to be present in every ES annotation JSON document.

Extracting the fixture will make it easier to add new fields to it.
Adjust indentation for nested dict literal
PEP 8 doesn't prescribe a style for nested dict literals but looking at
h's code I've gone with something that matches the output of
`jq --indent 4` as that seems reasonably consistent.
Remove empty selector list from annotation fixture
The `selector` field is not required and does not exist on page notes
for example.
Add an additional test case for the no-quote-selector scenario
An annotation may have selectors, just not a quote selector.
@fatbusinessman

This comment has been minimized.

Show comment
Hide comment
@fatbusinessman

fatbusinessman Dec 5, 2017

Cool. That all looks good to me; thanks for making those changes.

Either rebase your fixes into the original commits or not, as you see fit, and merge when ready.

fatbusinessman commented Dec 5, 2017

Cool. That all looks good to me; thanks for making those changes.

Either rebase your fixes into the original commits or not, as you see fit, and merge when ready.

@robertknight robertknight merged commit 3fec6f9 into master Dec 5, 2017

6 checks passed

codecov/patch 100% of diff hit (target 88.23%)
Details
codecov/project 88.44% (+0.21%) compared to 49ce1ad
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
hound No violations found. Woof!

@robertknight robertknight deleted the es-annotation-fixture-cleanup branch Dec 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment