Skip to content
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

test: add missing tests for 100% coverage #107

Merged
merged 3 commits into from
Jan 30, 2023

Conversation

adevade
Copy link
Contributor

@adevade adevade commented Jan 20, 2023

Description

Adds 3 tests to UrlHelperTest.php and 1 test to ValidatorTest.php. These tests brings the code base to 100% coverage.

testHelperFormatPathWithNoPath

Touches this piece of code in UrlHelper.php:

22 if (!is_string($path) || strlen($path) < 1)
23     return '/';

testHelperBuildSignedURLWithNullHashSetterParams & testHelperBuildSignedURLWithHashDeleterParams

Tests that unsetting parameters via setParameter() and deleteParameter() in UrlHelper.php works. It tests by first setting a value, and then either setting it to null or by using deleteParameter().


testValidateWidthsNegativeValues

Tests that negative values throws an exception in validateWidths().

The $widths == NULL on line 46 in Validator.php actually was truthy if $widths was an empty array.

testValidateWidthsEmptyArray() did therefore pass, but not for the right reason. It was passing because of the loose comparison in $widths == NULL was throwing the exception.

Link that shows the comparisons in action: https://3v4l.org/pWMCv
Clearer example: https://3v4l.org/JhR03


Changes to the code

The switch to strict comparisons (== to ===) just makes the code more "safe" by checking type as well as value.
The switch to is_null() in validateWidths() makes the code work as expected.

Before this PR

Run $ phpunit --coverage-text. Needs XDebug installed.

Code Coverage Report:      
  2023-01-20 14:41:09      

 Summary:
  Classes: 33.33% (1/3)    
  Methods: 83.33% (20/24)  
  Lines:   95.62% (131/137)

Imgix\UrlBuilder
  Methods: 100.00% (11/11)   Lines: 100.00% ( 73/ 73)
Imgix\UrlHelper
  Methods:  57.14% ( 4/ 7)   Lines:  90.91% ( 40/ 44)
Imgix\Validator
  Methods:  83.33% ( 5/ 6)   Lines:  90.00% ( 18/ 20)

After this PR

Run $ phpunit --coverage-text. Needs XDebug installed.

Code Coverage Report:       
  2023-01-20 14:42:11       
                            
 Summary:                   
  Classes: 100.00% (3/3)    
  Methods: 100.00% (24/24)  
  Lines:   100.00% (137/137)

Imgix\UrlBuilder
  Methods: 100.00% (11/11)   Lines: 100.00% ( 73/ 73)
Imgix\UrlHelper
  Methods: 100.00% ( 7/ 7)   Lines: 100.00% ( 44/ 44)
Imgix\Validator
  Methods: 100.00% ( 6/ 6)   Lines: 100.00% ( 20/ 20)

Checklist

@adevade adevade requested a review from a team as a code owner January 20, 2023 15:47
@commit-lint
Copy link

commit-lint bot commented Jan 20, 2023

Tests

  • add missing tests for 100% coverage (0692ba1)

Code Refactoring

  • use strict comparisons everywhere (0dae0e8)

Bug Fixes

  • catching empty arrays due to loose comparison (1234091)

Contributors

adevade

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

@luqven luqven added enhancement needs-triage added to the backlog labels Jan 20, 2023
@luqven luqven self-assigned this Jan 20, 2023
Copy link
Contributor

@luqven luqven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏼

Great work on this! Nice catch on the false positive.

@luqven luqven removed the needs-triage added to the backlog label Jan 30, 2023
@luqven luqven merged commit 68025cc into imgix:main Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants