Status

defect
ASSIGNED
5 years ago
2 years ago

People

(Reporter: marlio, Assigned: marlio)

Tracking

(Blocks 1 bug)

Details

Attachments

(2 attachments, 6 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee: nobody → malinthak2
Status: NEW → ASSIGNED
Assignee

Comment 1

5 years ago
Posted patch Test for imip-bar (obsolete) β€” β€” Splinter Review
This will be added to the usual thunderbird test directory mail/tests/mozmill with dependencies.
Attachment #8422549 - Flags: review?(philipp)

Comment 2

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

Comment 3

5 years ago
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.
Assignee

Comment 4

5 years ago
But I suggest this can be modified as a new patch for that bug.
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.
Attachment #8422549 - Flags: review?(philipp) → review-
> 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 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.
Assignee

Comment 8

5 years ago
Posted patch test-imip.patch (obsolete) β€” β€” Splinter Review
element.occurrence didn't work for me. appeared it didn't return any calendar property.
Attachment #8425171 - Flags: review?(philipp)
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.
Attachment #8425171 - Flags: review?(philipp) → review-
Assignee

Comment 10

5 years ago
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.
Assignee

Comment 11

5 years ago
Posted patch test-imip.patch (obsolete) β€” β€” Splinter Review
Added date/time assertion by getting values from event summary dialog box
Attachment #8422549 - Attachment is obsolete: true
Attachment #8425171 - Attachment is obsolete: true
Attachment #8470809 - Flags: review?(philipp)
Attachment #8470809 - Flags: feedback?(philipp)
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
Attachment #8470809 - Flags: review?(philipp)
Attachment #8470809 - Flags: review-
Attachment #8470809 - Flags: feedback?(philipp)
Assignee

Comment 13

5 years ago
Posted patch test-imip.patch v4 (obsolete) β€” β€” Splinter Review
Attachment #8470809 - Attachment is obsolete: true
Attachment #8484903 - Flags: review?(philipp)
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: 'CAGFBr6MY94cd_PFW5zxkAxjubHpW1OgsurjWdFKpZDmjh4VTaw@mail.gmail.com',

@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?
Attachment #8484903 - Flags: review?(philipp) → review-
Assignee

Comment 15

5 years ago
Posted patch test-imip.patch v5 (obsolete) β€” β€” Splinter Review
Attachment #8484903 - Attachment is obsolete: true
Attachment #8488986 - Flags: review?(philipp)
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
Attachment #8488986 - Flags: review?(philipp) → review-
Assignee

Comment 18

4 years ago
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?
Flags: needinfo?(philipp)
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 :-)
Flags: needinfo?(philipp)
Assignee

Comment 20

4 years ago
Posted patch test-imipV7.patch (obsolete) β€” β€” Splinter Review
Added asserting of StartTime and EndTime in native format after extracting the item from the node.
Attachment #8488986 - Attachment is obsolete: true
Attachment #8557529 - Flags: review?(philipp)
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.
Attachment #8557529 - Flags: review?(philipp) → review-
Assignee

Comment 22

4 years ago
Posted patch test-imipV8.patch β€” β€” Splinter Review
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.
Attachment #8557529 - Attachment is obsolete: true
Attachment #8565842 - Flags: review?(philipp)
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?
Attachment #8565842 - Flags: review?(philipp) → review+
Assignee

Comment 24

4 years ago
Posted patch test-imipV9.patch β€” β€” Splinter Review
Did the changes, now the event's being selected.
Attachment #8567424 - Flags: review?(philipp)
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
Attachment #8567424 - Flags: review?(philipp) → review-

Comment 26

3 years ago
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.
Flags: needinfo?(malinthak2)

Comment 27

3 years ago
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.
Depends on: 1309705
Flags: needinfo?(malinthak2)
You need to log in before you can comment on or make changes to this bug.