Closed Bug 1020574 Opened 10 years ago Closed 8 years ago

[mozmill] modernize calendar's sharedModules

Categories

(Calendar :: General, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1303626

People

(Reporter: Taraman, Assigned: Taraman)

References

Details

Attachments

(4 obsolete files)

Attached patch mozmillSharedModules.diff (obsolete) β€” β€” Splinter Review
The shared modules for the calendar Mozmill-Tests use some modules, which are not available in the comm-central repository.
These can be replaced by others used by Thunderbird in Mozmill-Testing thus enabling calendar tests to run as packaged tests.
Attachment #8434421 - Flags: review?(philipp)
Comment on attachment 8434421 [details] [diff] [review]
mozmillSharedModules.diff

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

r=philipp if you can present a green try-run with this applied :-)

::: calendar/test/mozmill/shared-modules/test-calendar-utils.js
@@ +7,4 @@
>  
> +Cu.import("resource://gre/modules/Services.jsm");
> +var os = {};      Cu.import('resource://mozmill/stdlib/os.js', os);
> +var frame = {};      Cu.import('resource://mozmill/modules/frame.js', frame);

I believe you can also write this as:

var frame = Cu.import('...', {});

@@ +59,5 @@
> + */
> +function getCalProperty(bundleName, propId) {
> +  var props = Services.strings.createBundle(
> +                 "chrome://calendar/locale/" + bundleName + ".properties");
> +  return props.GetStringFromName(propId);

Can't you use cal.calGetString for this?
Attachment #8434421 - Flags: review?(philipp) → review+
> I believe you can also write this as:
> var frame = Cu.import('...', {});

Yes, that works. 

And I present to you: A working try-run:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=56d4151e98ed
The failures on linux are not in our tests...

> Can't you use cal.calGetString for this?
There had been problems loading the calendar-modules in the tests. Meanwhile I believe it works.
For now I can leave this function out, because I will not need it until the timezone-tests are fixed. There I can try to use calGetString.

With this changed, I am going to push soon.
Attached patch patch V2 r=philipp (obsolete) β€” β€” Splinter Review
updated patch per the last comments

r=philipp
Attachment #8434421 - Attachment is obsolete: true
Attachment #8442871 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8442871 [details] [diff] [review]
patch V2 r=philipp

There are changes to be made to this patch for mac compatibility as I just found out.
Therefore this is obsolete and checkin-needed cancelled.
Attachment #8442871 - Attachment is obsolete: true
Attachment #8442871 - Flags: review+ → review-
Keywords: checkin-needed
Attached patch patch V2.1 (obsolete) β€” β€” Splinter Review
This is a patch with updated "setData" function.

I changed all the blocks containing "if (!mac)" statements to be platform independant.
For that, I took out combinations of "a + ctrlKey" since the ctrlKey does not work on Mac.
Instead, now all these parts contain ".select()" on the text-fields which also causes the whole text to be selected.

Surely this does not imitate the user interaction exactly, but since we are not testing for the behaviour of textboxes, I think this is acceptable.

I could not test the patch, because the builds are busted at the moment.
Philip, please test this when you have a working building env.
Unfortunately there are no tests working at the moment, which use this function, but we can make sure the module works overall.
Attachment #8443925 - Flags: review?(philipp)
Comment on attachment 8443925 [details] [diff] [review]
patch V2.1

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

I ran our three basic tests with only this patch applied locally and it seems to work. To be sure, I've started a try run too. 
I hope the recent bustages don't hinder the builds, I haven't updated in a while so we might be safe.

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

Code looks fine, r=philipp

If the try build passes, go ahead and push.
Attachment #8443925 - Flags: review?(philipp) → review+
Depends on: 1137643
Does this have importance in the effort to move to ical.js?
Flags: needinfo?(philipp)
I don't think it does. This would be nice cleanup for the mozmill tests though. I'm making some updates in bug 1303626 so this patch may need some updates.
Flags: needinfo?(philipp)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Attached patch Patch V2.2 (obsolete) β€” β€” Splinter Review
Debitrotted pacth after Bug 1297425
Attachment #8443925 - Attachment is obsolete: true
Attachment #8817340 - Flags: review?(makemyday)
I guess, this patch was intended for bug 1303626?
Uh, yes.
I was too tired and too quick yesterday...
Attachment #8817340 - Attachment is obsolete: true
Attachment #8817340 - Flags: review?(makemyday)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: