Closed BruceGoodGuy closed 1 year ago
The fix looks right to me.
However, the fact this bug exists says to me that our automated tests were not good enough. And, I don't see any chagnes in the automated tests which would prevent this, or a simiilar bug, happening again.
There are technicques to unit test message sending: https://docs.moodle.org/dev/Writing_PHPUnit_tests#Testing_sending_of_messages.
Also, it should not be necessary to us ob_start etc. You can use expectOutputString / expectOutputRegex.
Do you want to improve the tests here, or do you thing we should just merge the fix?
Hi @timhunt ,
I appreciate your feedback.
With your suggestion, I will enhance the unit test for the send email function.
As per your recommendation, I have updated the unit test in the file: tests/cron_test.php
(specifically test_send_no_digest_notification_task
and test_send_digest_notification_task
) to verify the output and email content.
I have also added a data provider to check the function in various question states. I hope that these fixes will make the unit test more efficient.
There is one more thing I would like to discuss with you regarding the CI result of the latest commit. After reviewing the CI results, I noticed a coding style issue in the file mod/studentquiz/lib.php
. Clearly, this issue does not belong to my commits. Should I fix it and push the changes in a separate commit?
Additionally, could you please review the changes again? Link to new commit: https://github.com/studentquiz/moodle-mod_studentquiz/pull/453/commits/84ee72494a63e87e1149f3d464522da21371f2c8
Thank you.
I accdientally merged the wrong whitespace recently. Sorry. There was Pull #457 to fix it which I just merged. So, I will now review this.
Thanks. Merged.
Hi @timhunt, I have adjusted lang string for question state. Could you please help me to preview it? Thanks.