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

Patch for #277: Fix bug and unit test for ExtractDomain #278

Merged
merged 2 commits into from Oct 17, 2018

Conversation

Projects
None yet
3 participants
@borislin
Collaborator

borislin commented Oct 16, 2018

This PR fixes the wrong order of url and source checking in ExtractDomain.


GitHub issue(s):

What does this Pull Request do?

This PR improves unit test for ExtractDomain to catch a bug where ExtractDomain mistakenly checks source first then url, and also fixes the bug by reversing the checking order of source and url.

How should this be tested?

  • Follow steps in #277 to reproduce the error and build will fail
  • git fetch --all
  • git checkout fix-extractdomain
  • mvn clean package
  • build should now pass

Interested parties

@lintool @greebie @ruebot

@borislin borislin requested a review from greebie Oct 16, 2018

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Oct 16, 2018

Codecov Report

Merging #278 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #278   +/-   ##
=======================================
  Coverage   70.36%   70.36%           
=======================================
  Files          41       41           
  Lines        1046     1046           
  Branches      192      192           
=======================================
  Hits          736      736           
  Misses        244      244           
  Partials       66       66
Impacted Files Coverage Δ
.../io/archivesunleashed/matchbox/ExtractDomain.scala 87.5% <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 4fe05a5...8150fc6. Read the comment docs.

codecov-io commented Oct 16, 2018

Codecov Report

Merging #278 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #278   +/-   ##
=======================================
  Coverage   70.36%   70.36%           
=======================================
  Files          41       41           
  Lines        1046     1046           
  Branches      192      192           
=======================================
  Hits          736      736           
  Misses        244      244           
  Partials       66       66
Impacted Files Coverage Δ
.../io/archivesunleashed/matchbox/ExtractDomain.scala 87.5% <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 4fe05a5...8150fc6. Read the comment docs.

@ruebot

This comment has been minimized.

Show comment
Hide comment
@ruebot

ruebot Oct 17, 2018

Member

@borislin can you update this branch?

Member

ruebot commented Oct 17, 2018

@borislin can you update this branch?

@ruebot

ruebot approved these changes Oct 17, 2018

Tested and good to go.

Just need to update the branch. Once that happens, I'll get this merged.

@ruebot ruebot merged commit 2c66b09 into master Oct 17, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ruebot ruebot deleted the fix-extractdomain branch Oct 17, 2018

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