[ZBX-8349] Failing unit tests for validation classes Created: 2014 Jun 12 Updated: 2017 May 30 Resolved: 2014 Jul 22 |
|
Status: | Closed |
Project: | ZABBIX BUGS AND ISSUES |
Component/s: | API (A), Frontend (F) |
Affects Version/s: | None |
Fix Version/s: | 2.3.3 |
Type: | Incident report | Priority: | Major |
Reporter: | Pavels Jelisejevs (Inactive) | Assignee: | Unassigned |
Resolution: | Fixed | Votes: | 0 |
Labels: | frontend, unittests, validation | ||
Remaining Estimate: | Not Specified | ||
Time Spent: | Not Specified | ||
Original Estimate: | Not Specified |
Description |
There are currently failing unit tests for validation classes. Some of the failures are results of actual validation bugs, others - results of incorrect implementation. We should also cover the remaining validators with unit tests. |
Comments |
Comment by Andrejs Čirkovs (Inactive) [ 2014 Jun 30 ] |
RESOLVED. Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-8349 r46934. Please note that remaining validators were not covered and incomplete tests were not written as it was discussed. Please review changes caused by messageInvalid -> messageType in CCollectionValidator. |
Comment by Andrejs Čirkovs (Inactive) [ 2014 Jun 30 ] |
(1) String added: NONE |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 01 ] |
(2) I get a bunch of strict standards errors when running the CLdapAuthValidatorTest:
Please enable all error reporting in bootstrap.php. andrewtch RESOLVED by r47005. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 01 ] |
(3) In CCollectionValidator: I think it would be better to rewrite // since second unique parameter can be absent, we call it in ugly way $params = array($this->messageDuplicate, $duplicate[$this->uniqueField]); if ($this->uniqueField2) { $params[] = $duplicate[$this->uniqueField2]; } call_user_func_array(array($this, 'error'), $params); as if ($this->uniqueField2 === '') { $this->error($this->messageDuplicate, $this->uniqueField); } else { $this->error($this->messageDuplicate, $this->uniqueField, $this->uniqueField2); } andrewtch RESOLVED by r47015. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 01 ] |
(4) The following test case in CCoolectionValidatorTest seems to be unnecessary: // this is a regression test // todo: rewrite messageInvalid to messageType array( array('messageType' => 'Not an array'), array( 'string', 100 ) ) andrewtch RESOLVED by r47015. Also added some test cases checking if uniqueField2 is null. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 01 ] |
(5) The CIdValidator should be rewritten so that it wouldn't extend the CStringValidator class. Otherwise it inherits too much unnecessary functions. Also, let's use a single error message for both regexp and range checks. RESOLVED in r47092. Now it's only messageInvalid. jelisejev I've made some changes in r47103, please review. andrewtch CLOSED, fine. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 01 ] |
(6) The "messageType" message should be added to uses of CSetValidator where it's required. andrewtch RESOLVED by r47027.
andrewtch RESOLVED (in a bit of ugly way) in r47161. jelisejev CLOSED. |
Comment by Andrejs Čirkovs (Inactive) [ 2014 Jul 01 ] |
(7) Add/complete CDecimalValidator class andrewtch RESOLVED in r47087. I have gathered all my courage and rewritten all the CDecimalValidator. Please also verify messageFormat -> messageType transitions. jelisejev I've made a minor improvement in r47107. andrewtch Thanks, fine. Should we also accept values with leading "+" sign, like +123? Otherwise, CLOSED. jelisejev Nah, "+" sign strings will not be supported. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 07 ] |
(8) I've changed the CSetValidator to treat true, false and decimals as invalid values. Please review my changes in r47106. andrewtch And what's the point? So it's not possible to find out if the value belongs to set of float values or is just "true", "false"? REOPENED. jelisejev Because the current implementation doesn't support boolean and decimal values. andrewtch So let's rename it to CLimitedSetValidator then. jelisejev Agreed. Please do that. andrewtch RESOLVED in r47153. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 09 ] |
(9) Another issue with the CDecimalValidator. if (!is_numeric($value) || !preg_match('/^-?\d+(\.\d+)?$/', $value)) { $this->error($this->messageType, $value); return false; } If we pass an array to this code, it will try to insert the array into a string, which is incorrect. We could solve this problem similarly to the CIdValidator: if a non-string and non-numeric value is passed, just trigger the "messageType" error without passing the value. But if it is a string or a number that doesn't match the regexp - trigger a "messageInvalid" error with the value. andrewtch I did it in a more elegant way in r47167 as introducing additional message would require a lot of time and refactoring / testing throughout a code. Please check; RESOLVED. jelisejev Good idea, I suggest to improve it: let's convert values to a matching string representation: arrays to "array", true to "true" etc. Something like: protected function getPrintableValue($value) { if (is_array($value)) { return 'array'; } elseif ($value === true) { return 'true'; } elseif ($value === false) { return 'false'; } elseif ($value === null) { return 'null'; } return $value; } Then we can remove "messageType" at all and just use "messageInvalid" for both type of errors instead. Note, that this needs to be done in all validators, not just this one. andrewtch RESOLVED in base CValidator r47260. I've also changed one test case to validate new behavior.
andrewtch RESOLVED in r47444. jelisejev CLOSED. |
Comment by Pavels Jelisejevs (Inactive) [ 2014 Jul 22 ] |
TESTED. |
Comment by Andrejs Čirkovs (Inactive) [ 2014 Jul 22 ] |
CLOSED. Fixed in pre-2.3.3 r47528. |