1010294 - mochitest test for IMIP bar

Status: NEW

(Calendar :: E-mail based Scheduling (iTIP/iMIP), defect)

Description

[Malintha Fernando [:marlio]]

There should be a mozmill test for IMIP bar. The test should inject a mail containing an invitation to check whether the IMIP bar is loaded and buttons are displayed. Then accept the event and make sure it is added at the correct location in the calendar view.

Comment 1

[Malintha Fernando [:marlio]]

Attachment: Test for imip-bar

This will be added to the usual thunderbird test directory mail/tests/mozmill with dependencies.

Comment 2

[[:MakeMyDay]]

Does this test also take about testing the imip bar buttons - see bug 1003207?

Comment 3

[Malintha Fernando [:marlio]]

No, This injects only an event invitation. So only Accept, Decline and Tentative buttons will be displayed and asserted. This test is meant to accept an event and assert it for adding it at the right time. Doesn't check the whole set of buttons.

Comment 4

[Malintha Fernando [:marlio]]

But I suggest this can be modified as a new patch for that bug.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8422549 [details] [diff] [review]
Test for imip-bar

Review of attachment 8422549 [details] [diff] [review]:
-----------------------------------------------------------------

There are a lot of files here we won't want to push at the end. The test-imip.js looks very good, but could you do some cleanup on the indentation and whitespaces? This guide is fairly old, but most of it still applies: https://wiki.mozilla.org/Calendar:Style_Guide

I'd appreciate if you could upload a patch that puts test-imip.js into a new directory calendar/test/mozmill/invitations/ and include the invitations directory in mozmilltests.list.

In the meanwhile, I will try to figure out what is needed to get the paths set up correctly.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

> There are a lot of files here we won't want to push at the end.
In case it was unclear, I mean all files except test-imip.js with this, as they are copies of files already in the tree.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8422549 [details] [diff] [review]
Test for imip-bar

Review of attachment 8422549 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I have figured out most of the mozmill loading issue, although I think we should ask Taraman too. He is doing some slight restrucutring in bug 980515 that we will eventually have to take into account. There is also an issue with the Thunderbird folder-display-helpers that make some assumptions as to where the mozmill files reside. I will look into fixing these.

In the meanwhile, you might want to download the packaged tests and use them to write your mozmill test instead. This should give you more sane path handling. I will email you with details soon.

Here is a second review, this time on test-imip.js:

::: mail/test/mozmill/calendar/test-imip.js
@@ +14,5 @@
> + 
> +var MODULE_NAME = 'test-imip';
> +var RELATIVE_ROOT = '../shared-modules';
> +var MODULE_REQUIRES = ['folder-display-helpers',
> +                       'window-helpers','calendar-utils','compose-helpers','prompt-helpers','timezone-utils'];

Make sure you only require the modules you really need.

@@ +23,5 @@
> +Cu.import('resource://mozmill/stdlib/EventUtils.js', EventUtils);
> +Cu.import("resource://gre/modules/Services.jsm");
> +var messenger;
> +var folder;
> +var os = {};

Same for elib, os, messenger, etc.

@@ +33,5 @@
> +				"VERSION:2.0",
> +				"CALSCALE:GREGORIAN",
> +				"METHOD:REQUEST",
> +        "BEGIN:VEVENT",
> +        "DTSTART:20140515T050000Z",

Be careful with timezones here. Depending on what default timezone is detected, the event box will appear in a different slot. I'd suggest to use local time (i.e just DTSTART:20140515T050000) and then use hour 5 in the test code below.

@@ +66,5 @@
> +  folder = create_folder('ImipFolder');
> +  messenger = Components.classes['@mozilla.org/messenger;1']
> +                        .createInstance(Components.interfaces.nsIMessenger);
> + 
> +  isWindows = '@mozilla.org/windows-registry-key;1' in Components.classes;

Also some unused variables here.

@@ +100,5 @@
> +
> +function test_attachment_view_collapsed() {
> +	be_in_folder(folder);
> +  select_click_row(0);
> +  assert_selected_and_displayed(0);    

This looks like preparation for one of the other tests, not a test for itself. You probably want to either add this to setupModule or to one of the other test functions.

@@ +120,5 @@
> +	}
> +	catch(e){
> +	//in a case throw up an error "cannot send mail", click ok on it 
> +	controller.window.alert(e);
> +	controller.keypress(undefined, "VK_ENTER", {});

Do we have a "cannot send mail" dialog here or is this copy/paste? Please be sure to clean it up.

@@ +125,5 @@
> +	}
> +		gMockPromptService.unregister();	
> +}
> +
> +function testInjectedEvent(){

A more general comment on structuring these tests: Per function you want to have a test that is mostly self-contained. You don't want testdeleteInjectedEvent to rely on what testInjectedEvent does.

You probably want to name your test "testAcceptREQUEST" and do both the accepting and removing there. Be sure to add some inline comments about what you are doing too (suggested granularity: "first open the message and make sure the imip bar shows up", "now accept the invitation into the calendar", "now switch to the calendar view and check if the event has been created")

@@ +130,5 @@
> +	//check for the event injected by mail
> +	controller.click(new elementslib.ID(controller.window.document, "calendar-tab-button"));
> + 	calUtils.switchToView(controller, "day");
> +	calUtils.goToDate(controller, 2014, 5, 15);
> +	controller.sleep(sleep);

For cleanup, remove the sleep statements unless you really need them. calendar-utils has a SLEEP variable that you can also use if you do need it.

@@ +133,5 @@
> +	calUtils.goToDate(controller, 2014, 5, 15);
> +	controller.sleep(sleep);
> +
> +	//make sure event was added to the right position
> +	controller.assertNode(new elementslib.Lookup(controller.window.document, calUtils.getEventBoxPath(controller,"day", calUtils.CANVAS_BOX,undefined, 1, 11)));

In addition to checking if its in the calendar, I'd suggest checking some more specific properties that should stay the same and/or must change when accepting into the calendar.

@@ +136,5 @@
> +	//make sure event was added to the right position
> +	controller.assertNode(new elementslib.Lookup(controller.window.document, calUtils.getEventBoxPath(controller,"day", calUtils.CANVAS_BOX,undefined, 1, 11)));
> +	}
> +	
> +function testdeleteInjectedEvent(){

This test didn't run for me on mac, although the other delete tests run. Does it work for you?

@@ +160,5 @@
> +}
> +
> +function testCreateEvent(){
> +	/*
> +	 * Adding the event manually

I don't think we need the test to create the event manually here, these tests should cover the invitation functionality only. I believe we have other views tests that should cover this.

Comment 8

[Malintha Fernando [:marlio]]

Attachment: test-imip.patch

element.occurrence didn't work for me. appeared it didn't return any calendar property.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8425171 [details] [diff] [review]
test-imip.patch

Review of attachment 8425171 [details] [diff] [review]:
-----------------------------------------------------------------

Please ping me on IRC so we can sort this out. With "element.occurrence" I meant the .occurrence property of the DOM node of the event box. If you do elementslib.lookup you get a mozmill wrapper, you will have to call getNode() on that.

Comment 10

[Malintha Fernando [:marlio]]

We need to place following helper classes still in the shared-modules directory. folder-display-helpers, window-helpers, compose-helpers, prompt-helpers. I assumed they are there to run the test.

Comment 11

[Malintha Fernando [:marlio]]

Attachment: test-imip.patch

Added date/time assertion by getting values from event summary dialog box

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8470809 [details] [diff] [review]
test-imip.patch

Review of attachment 8470809 [details] [diff] [review]:
-----------------------------------------------------------------

Please check for trailing whitespaces and make sure you use K&R style if/else brackets instead of Stroustrup. Also, it seems there are still some comments open from my previous review, please make sure they are taken care of (for example the comment about local time or naming).

::: calendar/test/mozmill/invitations/test-imip.js
@@ +13,5 @@
> +var calUtils = require('../shared-modules/calendar-utils');
> +var folder;
> +var os = {
> +};
> +Components.utils.import('resource://mozmill/stdlib/os.js', os);

var os = Components.utils.import('resource://...', {});

@@ +45,5 @@
> +/**
> + * initialize modules and inject the email with attachment
> + */
> +var jumlib = {}; 
> +Components.utils.import("resource://mozmill/modules/jum.js", jumlib);

Please put all imports at the top, and use same formatting as commented above.

@@ +120,5 @@
> +        promptState = gMockPromptService.promptState;
> +        controller.click(btnAccept);
> +    } 
> +    catch (e) {
> +    }

Please don't swallow exceptions.

@@ +158,5 @@
> +    let startAnonElem = event.window.document.getAnonymousElementByAttribute(start.getNode(), "anonid", "item-datetime-value");
> +    let startVal = startAnonElem.value;
> +    
> +    if(!(startVal=='Thu 15 May 2014 05:00 AM, UTC')){
> +        throw new Error("could not validate the values\n");

Instead of throwing an error, please use the mozmill assertion functions.

@@ +178,5 @@
> +/**
> + * Delete the event from calendar
> + */
> +
> +function testDeleteEvent() {

This is cleanup for the previous test, right? It should't be a new function, otherwise its like adding a new sub-test for this file.

@@ +188,5 @@
> +        promptState = gMockPromptService.promptState;
> +        controller.keypress(new elementslib.ID(controller.window.document, 'day-view'), 'VK_DELETE', {
> +        });
> +    } catch (e) {
> +    }

Don't swallow the exception

Comment 13

[Malintha Fernando [:marlio]]

Attachment: test-imip.patch v4

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8484903 [details] [diff] [review]
test-imip.patch v4

Review of attachment 8484903 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't actually run this test due to the current failures, but I think we are almost there. Just a few more minor comments. Feel free to ping me on IRC if you'd like to discuss anything.

::: calendar/test/mozmill/invitations/test-imip.js
@@ +1,1 @@
> +    /* This Source Code Form is subject to the terms of the Mozilla Public

Is it just how its displayed, or is the whole file indented by one tab?

@@ +81,5 @@
> +                    ],
> +                    contentType: 'text/calendar',
> +                    MIMEVersion: '1.0',
> +                    method: 'REQUEST',
> +                    messageId: '[e-mail]',

@example.com

@@ +94,5 @@
> +    }
> +
> +    /**
> +     * move to the folder, display the message and check whether IMIP bar accept button is showed up.
> +     * try{}/catch{} is for, accepting any exceptions throw when accepting the invitation by trying to send out emails.

There is no more try/catch, please update the comment.

@@ +110,5 @@
> +        //to return the value for not send email notifications button on the dialog
> +        gMockPromptService.returnValue = 1;
> +        promptState = gMockPromptService.promptState;
> +        controller.click(btnAccept);
> +        gMockPromptService.unregister();

Maybe you can assert a little more here. I think when the event is accepted, the bar changes and shows the details button?

Comment 15

[Malintha Fernando [:marlio]]

Attachment: test-imip.patch v5

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Doing a tryserver run to see how this goes:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9460288e667b

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8488986 [details] [diff] [review]
test-imip.patch v5

Unfortunately the tests don't pass on some platforms. Could you check the test failures? 

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b1f015505939

Comment 18

[Malintha Fernando [:marlio]]

Reason behind failing was different platforms showing the start/end time in the event summary dialog box in different formats. Is there any way to format the date/time string from different formats to a standard format?

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

Removing needinfo, as we discussed this via IRC. The idea is to just get the occurrence from the item and figure out if it technically was added to the calendar. The focus is the imip bar, not the views the item is shown in. Let me know if you have more questions :-)

Comment 20

[Malintha Fernando [:marlio]]

Attachment: test-imipV7.patch

Added asserting of StartTime and EndTime in native format after extracting the item from the node.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8557529 [details] [diff] [review]
test-imipV7.patch

Review of attachment 8557529 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few more comments:

::: calendar/test/mozmill/invitations/test-imip.js
@@ +96,5 @@
> +
> +/**
> + * move to the folder, display the message and check whether IMIP bar accept button is showed up.
> + */
> +function testAcceptREQUEST() {

This function (and the next) could use a few blank lines for readability. Just add them in like you would paragraphs in real text.

@@ +141,5 @@
> +    let startTime = eventNodes[0].mOccurrence.startDate.nativeTime;
> +    let endTime = eventNodes[0].mOccurrence.endDate.nativeTime;
> +    // assert start and end times in native format
> +    assert_equals(startTime, '1400130000000000');
> +    assert_equals(endTime, '1400135400000000');

Any reason you are comparing with a string and not a number?

@@ +150,5 @@
> +    }, sleep);
> +    let event = new mozmill.controller.MozMillController(mozmill.utils.getWindows('Calendar:EventSummaryDialog') [0]);
> +    // check title
> +    let title = new elementslib.ID(event.window.document, 'item-title');
> +    controller.assertValue(title, 'IMIP testing event');

You don't need to open the event dialog at all, you can assert all the properties directly on eventNodes[0].mOccurrence. Might want to create a variable for that.

@@ +153,5 @@
> +    let title = new elementslib.ID(event.window.document, 'item-title');
> +    controller.assertValue(title, 'IMIP testing event');
> +    // check organizer
> +    let organizer = new elementslib.ID(event.window.document, 'item-organizer');
> +    controller.assertValue(organizer, 'Organizer');

We should also assert for the attendees (on mOccurrence also, of course)

@@ +166,5 @@
> +    gMockPromptService.returnValue = 1;
> +    calUtils.goToDate(controller, 2014, 5, 15);
> +    promptState = gMockPromptService.promptState;
> +    controller.keypress(new elementslib.ID(controller.window.document, 'day-view'), 'VK_DELETE', {
> +    });

please put the }); on the previous line.

Comment 22

[Malintha Fernando [:marlio]]

Attachment: test-imipV8.patch

Last time when I was comparing two strings extracted from the UI, the test had failed, due to different representations of time on different systems. So, for the convenience I used native time format here.

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8565842 [details] [diff] [review]
test-imipV8.patch

Review of attachment 8565842 [details] [diff] [review]:
-----------------------------------------------------------------

Codewise this looks fine with the following nits. I'm having trouble running the tests, so we might have to do a try-run.

::: calendar/test/mozmill/invitations/test-imip.js
@@ +145,5 @@
> +    let eventNodes = new Array();
> +
> +    // since we inject only one event, get the first node of the array
> +    calUtils.findEventsInNode(eventNode, eventNodes);
> +    let mItem = eventNodes[0].mOccurrence;

The "m" prefix means "member". This is not a member, so just "item" is better.

@@ +152,5 @@
> +    let startTime = mItem.startDate.nativeTime;
> +    let endTime = mItem.endDate.nativeTime;
> +    assert_equals(startTime, '1400130000000000');
> +    assert_equals(endTime, '1400135400000000');
> +    

Some extra whitespaces snuck in here.

@@ +180,5 @@
> +    gMockPromptService.register();
> +    gMockPromptService.returnValue = 1;
> +    calUtils.goToDate(controller, 2014, 5, 15);
> +    promptState = gMockPromptService.promptState;
> +    controller.keypress(new elementslib.ID(controller.window.document, 'day-view'), 'VK_DELETE', {});

Is the event still selected at this point?

Comment 24

[Malintha Fernando [:marlio]]

Attachment: test-imipV9.patch

Did the changes, now the event's being selected.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8567424 [details] [diff] [review]
test-imipV9.patch

Review of attachment 8567424 [details] [diff] [review]:
-----------------------------------------------------------------

Did you run the tests locally? I'm getting this error:

SUMMARY-UNEXPECTED-FAIL | test-imip.js | test-imip.js::setupModule
  EXCEPTION: Could not find [object Object] in available paths
    at: test-folder-display-helpers.js line 2830
       load_via_src_path test-folder-display-helpers.js:2830 9
       setupModule test-folder-display-helpers.js:121 3
       installInto test-folder-display-helpers.js:261 3
       setupModule test-imip.js:49 5
       Runner.prototype.wrapper frame.js:580 7
       Runner.prototype._runTestModule frame.js:629 5
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3
SUMMARY-SKIP | test-imip.js::testAcceptREQUEST
SUMMARY-SKIP | test-imip.js::testEvent

Comment 26

[[:MakeMyDay]]

Malintha, are you still working on getting this mozmill test to be ready for landing?

I recently ran into the same problem with including folder-display-helper in a calendar test, so I was wondering if you have resolved that issue for your patch already.

Comment 27

[[:MakeMyDay]]

Just a heads up: For the test failure with folder-display-helpers I filed bug 1309705. If you still working on this, watch also bug 1225784, where I am implementing also tests for email invitations.

Comment 28

[Magnus Melin [:mkmelin]]

mozmill is gone, replaced by mochitest