511193 - [Mozmill] Recursion tests

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Merike Sell (:merike)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; et; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729)
Build Identifier: 

Tracking bug.

Reproducible: Always

Comment 1

[Merike Sell (:merike)]

Attachment: Daily recursion

Modified two handle functions because waiting for notification dialog triggers a failure if there isn't any.

Other functions added to CalendarUtils are for convenience.

Diff against bug 500469 attachments.

Comment 2

[Merike Sell (:merike)]

Attachment: Daily recursion

Previous patch suffered from a missing licence plate, inappropriate test function name and needlessly long sleep time. Here's a better one :).

Comment 3

[cmtalbert]

I'll jump on this today.  Sorry for the delay.

Comment 4

[Merike Sell (:merike)]

Attachment: Daily recursion

That was not better, wrong file. Sorry for bugspam.

Comment 5

[cmtalbert]

Comment on attachment 396272 [details] [diff] [review]
Daily recursion

Wow, this looks really good.  A few nits...

>+  // pick day
>+  controller.click(new elementslib.Lookup(controller.window.document, miniMonth
>+    + 'anon({"anonid":"minimonth-calendar"})/[' + (dateRow + 1) + ']/['	+ dateColumn + >']'));
There is a tab in this line ^^ (before + dateColumn).  Can you remove it?

>+
>+/**
>+ *  @param view - day, week, multiweek or month
>+ *  @param option - bg for creating event, fg for checking
>+ *  @param row - only used in multiweek and month view, 1-based index of a row
>+ *  @param column - 1-based index of a column
>+ *  @param hour - index of hour box
>+ *  @param controller - main window controller
>+ *  @returns path string
>+ */
What do the acronyms "bg" and "fg" have to do with "create" and "check" respectively?  Can we use something a little more clear here?  Like "CREATE" and "CHECK"?  I'm curious to know why you chose these and if there's a reason for it that I'm missing.

Outside of that, a stellar test.  r=ctalbert

Comment 6

[Merike Sell (:merike)]

(In reply to comment #5)
> (From update of attachment 396272 [details] [diff] [review])
> >+    + 'anon({"anonid":"minimonth-calendar"})/[' + (dateRow + 1) + ']/['	+ dateColumn + >']'));
> There is a tab in this line ^^ (before + dateColumn).  Can you remove it?

Of course, I always end up with some tabs with a new Notepad++ install before disabling them.

> >+ *  @param option - bg for creating event, fg for checking
> What do the acronyms "bg" and "fg" have to do with "create" and "check"
> respectively?  Can we use something a little more clear here?  Like "CREATE"
> and "CHECK"?  I'm curious to know why you chose these and if there's a reason
> for it that I'm missing.

I didn't have any good ideas what to call it. Basically to create an event you need to click on a background box while event itself is in foreground. I think "CREATE" would be ok, but "CHECK" isn't the best as you'd need the same path when opening the event, making it active for deleting and rightclick actions.

Comment 7

[cmtalbert]

(In reply to comment #6)
> 
> I didn't have any good ideas what to call it. Basically to create an event you
> need to click on a background box while event itself is in foreground. I think
> "CREATE" would be ok, but "CHECK" isn't the best as you'd need the same path
> when opening the event, making it active for deleting and rightclick actions.
Ok, it wasn't obvious that you are making the distinction between background and foreground event boxes.  I think that calling out that this is the distinction you're making and that you use bg for grabbing a panel to create an event and fg for checking an existing event.  Perhaps you could make this into constants so that the behavior is more clear.  I think that we'll be reusing a lot of this recursion code, so making it easy to read would be really useful.

Perhaps:
const FG_EVENT_BOX = 0; // Use when you need a foreground event box
const BG_EVENT_BOX = 1; // Use when you need a background event box, or the calendar canvas itself for a particular view

Then change your functions to accept those.

Do those seem to capture the essence of your names?

Comment 8

[Merike Sell (:merike)]

Attachment: Daily recursion 2

Patch to daily recursion test. Removing tabs and adding constants.

Comment 9

[Merike Sell (:merike)]

Attachment: Weekly recursion II

Comment 10

[Merike Sell (:merike)]

Attachment: Weekly recursion II, III and IV

Replacing previous patch with a new one that doesn't include some mistakes I was able to catch and adds two more tests.

Comment 11

[Merike Sell (:merike)]

Attachment: Monthly I and annual recursion

Comment 12

[Merike Sell (:merike)]

Attachment: Weekly recursion I

I'm not quite happy about how lengthy it turned out, so if there are any suggestions for generalizing or simplification then I'm all for it.

Also, caught one missing line from a previous test.

Comment 13

[cmtalbert]

(In reply to comment #12)
> Created an attachment (id=401501) [details]
> Weekly recursion I
> 
> I'm not quite happy about how lengthy it turned out, so if there are any
> suggestions for generalizing or simplification then I'm all for it.
> 
> Also, caught one missing line from a previous test.

Yeah this is a bear of a test.  I don't see any way to generalize it per se.  I also think this patch is missing a change to calendarUtils w.r.t. EVENT_BOX  I don't see that ever getting added to calendarUtils in any of these patches -- is it possible my tree is out of date?

Also, I can't get any of these tests to run on my build (nor on a nightly) -- looks like something mozmill is doing is throwing an error, but i can't figure out why.  Are you able to run these?  I assume so.  

Can you get some new patches diffed against the code in comm-central/calendar/test/mozmill/ that way the patches will easily apply and I can be sure I didn't mess something up in applying them by hand.

I looked at them all, and they look good, except for those missing bits of calendarUtils that don't seem to be included in any of these patches.

You can attach them all in one big patch that applies against the current directory structure.  Overall they look good, but I want to see them run before I r+.
Thanks!

Comment 14

[Merike Sell (:merike)]

Attachment: All tests against comm-central

EVENT_BOX is added in attachment 397080 [details] [diff] [review] which addresses comments on attachment 396272 [details] [diff] [review].

Attaching diff against comm-central. All run for me.

Comment 15

[Merike Sell (:merike)]

Forgot to add that the remaining monthly recursions depend on bug 514944 as daypicker elements and recurrence preview elements seem unavailable to Mozmill. There's no bug on preview elements but as the error is the same, solving the one open should provide either a solution or other useful and needed information.

Comment 16

[Merike Sell (:merike)]

Attachment: Recursion tests in rotated view

Those are nearly the same as recursion tests. Added switching to rotated view at the beginning of tests and rotation reset at the end of tests.

Comment 17

[cmtalbert]

Comment on attachment 399312 [details] [diff] [review]
Monthly I and annual recursion

>--- /dev/null
>+++ b/testCalendar/testRecursion/testAnnualRecursion.js

>+  let checkYears = [startYear, startYear + 1, epoch - 1, epoch, epoch + 1];
>+  let box = "";
>+  for(let i = 0; i < checkYears.length; i++){
I got some weird timing issues at the beginning of this for loop.  The test to match the event box would fail sporadically.  Once I ensured that things were ok and put a little 500ms sleep at the beginning of the loop, things seemed a bit better for me and there were no spurious assertion failures. Do you see anything like that?  I'm not sure what to recommend.  I'd rather not throw sleeps in if we don't need them.  Perhaps just a comment noting this so that in case we see the issue later we know where to start looking.

>diff --git a/testCalendar/testRecursion/testLastDayOfMonthRecursion.js b/testCalendar/testRecursion/testLastDayOfMonthRecursion.js
>new file mode 100644
>--- /dev/null
>+++ b/testCalendar/testRecursion/testLastDayOfMonthRecursion.js
>@@ -0,0 +1,153 @@
this test worked beautifully.

Nice!!

Comment 18

[cmtalbert]

Comment on attachment 397674 [details] [diff] [review]
Weekly recursion II, III and IV

>diff --git a/testCalendar/testRecursion/testBiweeklyRecursion.js b/testCalendar/testRecursion/testBiweeklyRecursion.js

>+  // delete event
>+  controller.click(new elementslib.Lookup(controller.window.document, box));
>+  CalendarUtils.handleParentDeletion(false);
>+  controller.keypress(new elementslib.ID(controller.window.document, "month-view"),
>+    "VK_DELETE", {});
Test runs great, but I see a bunch of events still existing after the test runs.  It'd be nice to remove all of those after the test run completes just to ensure that we don't get unexpected dialogs if we run the test twice on the same calendar.  Perhaps you could create a generic "delete all events" on the calendar call in your calUtils module that is called as part of a teardown step for each test. I think the easiest way to do that might be to simply use the XPCOM interfaces to remove and create the "mozmill" calendar after each test.

>diff --git a/testCalendar/testRecursion/testWeeklyNRecursion.js b/testCalendar/testRecursion/testWeeklyNRecursion.js
This one runs great.

>diff --git a/testCalendar/testRecursion/testWeeklyUntilRecursion.js b/testCalendar/testRecursion/testWeeklyUntilRecursion.js
>new file mode 100644
>--- /dev/null
>+++ b/testCalendar/testRecursion/testWeeklyUntilRecursion.js

>+const sleep = 500;
>+var calendar = "Mozmill";
>+var endDate = "26.01.2009"; // last Monday in month
This is going to get interesting.  If I run your test with the European style date/time then I get my "Until" date set to Feb 2, 2011, which is obviously not going to work on the test. We need to detect whether we are using MM.DD.YYYY or DD.MM.YYYY style dates and use the appropriate one for the end Date.  Once I made this change, the test worked great.

I poked around our code for a little while this morning.  I know we inherit that setting from the OS settings, but I can't find where we do that.  Philipp will likely know.

r+ with the change to detect the date format style.

Comment 19

[cmtalbert]

Comment on attachment 401501 [details]
Weekly recursion I

>diff --git a/testCalendar/testRecursion/testWeeklyWithExceptionRecursion.js b/testCalendar/testRecursion/testWeeklyWithExceptionRecursion.js
>new file mode 100644
>+  event.keypress(startDate, "a", {ctrlKey:true});
>+  event.type(startDate, "06.01.2009");
Here again, the test fails on US style dates because you reset the Monday to occur on June 1 on a MM/DD/YYYY style system, therefore later when you check to get two instances of the recurrence on Tuesday, the test fails.  We need to detect the style of date here and format this properly then the test will work.

r+ with that change.

Comment 20

[cmtalbert]

Comment on attachment 405572 [details] [diff] [review]
Recursion tests in rotated view

These look good, but they also have the date format problem mentioned above.  r+ with that same change.

Sorry it took so long to get to these reviews.

Comment 21

[Merike Sell (:merike)]

(In reply to comment #18)
> (From update of attachment 397674 [details] [diff] [review])
> Test runs great, but I see a bunch of events still existing after the test
> runs.  It'd be nice to remove all of those after the test run completes just to
> ensure that we don't get unexpected dialogs if we run the test twice on the
> same calendar.
Odd. I have no events after it finishes. Nothing in error console?

> Perhaps you could create a generic "delete all events" on the
> calendar call in your calUtils module that is called as part of a teardown step
> for each test. I think the easiest way to do that might be to simply use the
> XPCOM interfaces to remove and create the "mozmill" calendar after each test.
That would be great indeed, in case deletion using UI fails it would ensure that following tests are not affected. I guess this would go into teardownTest().

Comment 22

[Merike Sell (:merike)]

Attachment: Follow-up patch

Can be applied on top of patch 402625 and 405572.

Fixes:
- entering dates dependent on locale
- creating test calendar at the beginning of the test and deleting it at the end
- removing sleep(0) calls which are not needed any more

Comment 23

[cmtalbert]

Comment on attachment 407831 [details] [diff] [review]
Follow-up patch

Looks great.  Do you need me to check this in for you?

Comment 24

[Merike Sell (:merike)]

(In reply to comment #23)
> (From update of attachment 407831 [details] [diff] [review])
> Looks great.  Do you need me to check this in for you?

Yes.

Comment 25

[cmtalbert]

Checking in:
* Attachment 402625 [details] - changeset a4a14d177640
* Attachment 405572 [details] [diff] - changeset e7619c74e2f6
* Attachment 407831 [details] [diff] - changeset eaa8dc210e53

--> FIXED