Code review comment for lp:~nik90/ubuntu-clock-app/edit-alarm-qml-tests

Revision history for this message
Leo Arias (elopio) wrote :

85 + Usage:
86 + Utils {
87 + id: testUtils
88 + }

I find it weird to import the test utilities this way. Is it a common practice in QML?
I'm wondering if this would be better as the parent test case. Name the file ClockTestCase.qml, and then instead of UbuntuTestCase, you make all your tests extend ClockTestCase.
That has some downsides, because we will be making something like an inheritance hierarchy, which sometimes is bad. What you have here is more like composition, but feels weird because of the declarative paradigm. I'm not sure, just throwing the idea here. Feel free to do it however you think it's best.

102 + testUtils._pressHeaderButton(header, "addAlarmAction")

You have a typo here. Now it doesn't have the leading underscore.

144 + // Click on the clear button shown on the right
145 + mouseClick(textfield, textfield.width - units.gu(2), centerOf(textfield).y)

Wouldn't it be better to find the clear button and then click its center? It should have an objectName.

230 + function _confirmListItemValue(page, objectName, expectedValue, message) {

Instead of confirm, we generally use the word "assert". check or compare also sound better than confirm to me.

321 + compare(Qt.formatTime(alarmTimePicker.date), oldtime, "Time read from the saved alarm is incorrect")
324 + _confirmListItemValue(addAlarmPage, "alarmRepeat", oldrepeat, "Alarm repeat options read from the saved alarm is incorrect")
332 + _confirmListItemValue(addAlarmPage, "alarmLabel", oldlabel, "Alarm name read from the saved alarm is incorrect")
340 + _confirmListItemValue(addAlarmPage, "alarmSound", "Celestial", "Alarm sound read from the saved alarm is incorrect")

Generally, you shouldn't do assertions in the middle of the test steps. Why would the alarm now have an incorrect value? If you have a test that makes sure that the alarm was created correctly where you are already doing these assertions, there's no need to repeat them here. We can assume that the alarm creation is a tested unit of code that will always work. If you haven't done those assertions on a previous test or you can't rely on the alarm creation tests to fail if some of your assumptions for this test break, then you need to work a little more on the alarm creation tests.

391 + #NOTE: This wait is required since as per the design after an alarm is edited and saved
[...]

Awesome, great comment! Bonus points.

403 + _deleteAlarm("Alarm Edited", "Weekends", Qt.formatTime(newDate), true)

I saw this on a previous branch but forgot to comment about it. There's a chance that it won't run, if a previous step fails. So you could end up with an alarm that affects the rest of the tests. As we don't have an addCleanup in QML tests, is there an easy way to delete all the existing alarms (if any) on the cleanup function?
Or, are you planning to mock the alarm data in the short term, so is it better to spend the time doing the mock instead of improving the cleanup?

This looks great. As always, many things are just opinions so feel free to disagree. I just would like to see the typo fixed; and the assertions at the end of a test instead of in the middle of the steps. That is, unless you have a good reason to do it.

review: Needs Fixing

« Back to merge proposal