-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
[WIP] Validator tests #683
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
Conversation
|
Thank you for adding test cases. Your effort is greatly appreciated! Why adding |
|
Yes, we need to, but I couldn't think of a clean solution to do this, without tapping into view or asset management to deep for now, I'll focus on the other tests and remove that tag later. |
|
Changes Unknown when pulling 467d88f on suralc:add-tests into * on yiisoft:master*. |
1 similar comment
|
Changes Unknown when pulling 467d88f on suralc:add-tests into * on yiisoft:master*. |
|
Changes Unknown when pulling f7bb54b on suralc:add-tests into * on yiisoft:master*. |
|
Sorry for the long delay, I'll add the remaining tests asap (this week) |
|
No problem. Your contribution is always welcome. Thank you. 👍 |
|
@qiangxue If a validator is not associated with any attributes it will raise a php-error if Validator::validate($object) is called. Situation may never occur in user-code as there will be an exception earlier in Model::createValidators(). However, if a validator is created by hand and one use it via Validator::validate($obj) he might run into this undeclarative error: Should this stay as is, or raise an exception? |
|
Fixed in 1ca215d |
I think Validator::$attributes should be empty array by default |
|
@qiangxue :-D great, same thought in same second ;) |
|
Would be the most obvious ;) Thanks |
|
Coverage remained the same when pulling 4727049c3a364b18a590a7ee8e3edc20868207fe on suralc:add-tests into 6e32fae on yiisoft:master. |
|
Coverage increased (+0%) when pulling 4ffffec328931f85ad2b07c8a5fe794081869fcb on suralc:add-tests into 6e32fae on yiisoft:master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about naming, or if this is to be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems we don't need it. You can assign ActiveRecord::$db with $this->getConnection(), like ActiveRecordTest.
|
Coverage increased (+2.73%) when pulling c336c7b2b83234a2480cffd0135da3de711f2bc3 on suralc:add-tests into 9a9a9c0 on yiisoft:master. |
|
@suralc thanks for the great work in here. can you tell whats missing? |
|
Updated and merged. @suralc thank you very much! |
Begun to write some tests for the validators.
I'll add additional tests in the comming days. I just opened this PR, that no duplicate work is made.