moodlehq / moodle-performance-comparison

Set of shell scripts to run Moodle performance tests using different hardware and configurations and compare results.
GNU General Public License v2.0
75 stars 39 forks source link

Failing due to invalid comparison from JSON #61

Closed andrewnicols closed 6 years ago

andrewnicols commented 6 years ago

58/#59 included a fix to remove non-numeric values (https://github.com/moodlehq/moodle-performance-comparison/pull/59/commits/a902b3276bd94397b14d5cbda8c27e4b0a4d4328)

However, in some situations, the value is fully-formed JSON and is now being altered in a breaking way:

{"bystep":{"dbreads":1,"dbwrites":2000,"dbquerytime":2000,"memoryused":3,"filesincluded":1,"serverload":2000,"sessionsize":2,"timeused":40},"total":{"dbreads":1,"dbwrites":2000,"dbquerytime":200,"memoryused":3,"filesincluded":1,"serverload":20000,"sessionsize":2,"timeused":6}}

becomes:

12000200031200024012000200312000026
andrewnicols commented 6 years ago

David's description of the breaking change was:

The change in webapp/classes/properties_reader.php cleans a trailing space

Switching this to use trim instead should deal with the trailing space, but without killing off the JSON entirely.

dmonllao commented 6 years ago

Thanks Andrew.

https://github.com/moodlehq/moodle-performance-comparison/commit/a902b3276bd94397b14d5cbda8c27e4b0a4d4328#diff-cd8e86ad10666ad53759ff7462da06b2R46 was an unrelated innocent tiny change I sneaked into that issue fixed because I noticed I had a space that was breaking my output; trim behaviour is what I wanted, I don't know why I limited it to numbers but what a glorious day... Anyway, your patch looks good and fixes the problem, we could apply the same trim to https://github.com/moodlehq/moodle-performance-comparison/blob/b2fb3af9144427bba3ec57a3f958549a447f4a33/webapp/classes/report.php#L359 but it is less likely that people introduce spaces as a json string is passed (they should set it as a string).

Up to you. I am happy to push the patch as it is. Waiting for your reply to act.

stronk7 commented 6 years ago

Tangentially... #58 should be also closed, I bet...