Write Some Assertions for Validation Library #10582
Conversation
| @@ -2813,6 +2817,9 @@ public function testContainNonAlphaNumeric() | |||
| $this->assertFalse(Validation::containsNonAlphaNumeric('abcdef##', 3)); | |||
| $this->assertFalse(Validation::containsNonAlphaNumeric('abc##def', 3)); | |||
| $this->assertFalse(Validation::containsNonAlphaNumeric('ab#cd#ef', 3)); | |||
|
|
|||
| //CHECK SCALAR | |||
There was a problem hiding this comment.
Why are these comments in ALL CAPS?
There was a problem hiding this comment.
No special reason. Should I make it capitalize or lowercase?
There was a problem hiding this comment.
I think so. Cause someone might be mistakenly tried with Scalar value (CC or AlphaNumeric).
There was a problem hiding this comment.
But arent you doing the opposite here? Checking a non-scalar (array) value?
Usually one can make a separate test method with the method name making it clear, also optionally adding a doc block note there.
There was a problem hiding this comment.
So, what do you suggest here?
There was a problem hiding this comment.
testContainNonAlphaNumericWithArray() etc for example.
There was a problem hiding this comment.
OK... Please do me a favor. I will do something like that you said. So should I send another pull request after modify it? And If so then what about my current pull request?
There was a problem hiding this comment.
You can add commits to your branch. This PR will automatically be updated :)
Codecov Report
@@ Coverage Diff @@
## master #10582 +/- ##
============================================
+ Coverage 94.86% 94.89% +0.02%
Complexity 12101 12101
============================================
Files 422 422
Lines 30073 30072 -1
============================================
+ Hits 28530 28537 +7
+ Misses 1543 1535 -8
Continue to review full report at Codecov.
|
…h scalar (array) of Validation Library
| @@ -3104,4 +3099,10 @@ public function testImageWidth() | |||
| $this->assertFalse(Validation::imageWidth($upload, '>', 2000)); | |||
| $this->assertFalse(Validation::imageWidth($upload, '==', 3000)); | |||
| } | |||
|
|
|||
| public function testContainNonAlphaNumericAndCCWithScalar() | |||
There was a problem hiding this comment.
Why scalar? Those are non scalar values - also the 2nd one is not the same as in the previous commit (it now is a scalar).
There was a problem hiding this comment.
I will fix it.
| @@ -168,6 +168,7 @@ public function testLengthBetween() | |||
|
|
|||
| $this->assertFalse(Validation::lengthBetween('abcdefg', 1, 6)); | |||
| $this->assertFalse(Validation::lengthBetween('ÆΔΩЖÇ', 1, 3)); | |||
| $this->assertFalse(Validation::lengthBetween(1, 1, 3)); | |||
There was a problem hiding this comment.
I feel like this should be true actually - is there maybe a cast missing? Or are we ok with the string input only for it?
There was a problem hiding this comment.
No casting issue. It's just take string input.
| public function testContainNonAlphaNumericAndCCWithScalar() | ||
| { | ||
| $this->assertFalse(Validation::cc(['869972521242198'], ['voyager'])); | ||
| $this->assertFalse(Validation::containsNonAlphaNumeric('ab#cd#ef', 3)); |
There was a problem hiding this comment.
Why is this here vs. with the other tests for this method?
There was a problem hiding this comment.
So...What do you suggest?
There was a problem hiding this comment.
These tests could be added to the existing test methods.
There was a problem hiding this comment.
OK... I will modify it.
…ove the Assertion to existing test methods
…) method. (mistakenly removed)
|
I am still not sure any of the tests brings value. They are either covered or completely out of scope (passing an array never works by definition). |
|
Thanks @sohelrana820 |
No description provided.