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

@elopio, thnx a lot for the detailed review. All the points you raised were very valuable and I have addressed them in the later revisions.

> 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.

Excellent Idea! To be honest I am unable to think why I chose the Utils{} approach rather than extending the UbuntuTestCase{} to ClockTestCase{}.

> 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.
>

I swear I looked at the Ubuntu SDK qml test case and couldn't find it earlier. But today I found it after some more digging. Hmm with AP tests this all was written by you, while in QML I am essentially rewriting the SDK Qml Test helpers for clock app from scratch ;)

> 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.

I renamed the functions to assert..() and also explained why those assert statement are required. I am essentially try to prevent bug 1338697 from happening again. While Bug 1338697 was certainly due to an upstream revision, it affected clock app and should be detected appropriately by our qml tests. Call me paranoid when it comes to testing alarm functionality.

>
> 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?

Ah that's a good point. The issue is that since clock uses EDS, it is not easy to mock the alarms data especially when using the Alarms API. I have explained this in bug 1283031. However say the deleteAlarm() function doesn't get called due to failure in the previous steps, it will *not* fail the other tests since they look specifically for the alarm it created rather than just checking the first alarm of the list for confirmation.