948065 - Mochitest for nsIClipboard

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

Add mochitest to test the basic functions of nsIClipboard.

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: mochitest v1.patch

Comment 2

[Yuan Xulei [:yxl] (Leave from May 18)]

https://tbpl.mozilla.org/?tree=Try&rev=32506e1d0ba7

Comment 3

[(no longer active)]

Comment on attachment 8344819 [details] [diff] [review]
mochitest v1.patch

Clipboard access is not necessarily synchronous.  We have a helper in our testing framework for this: SimpleTest.waitForClipboard.

I'm not sure what this is trying to test.  Why is this not covered by our existing tests?

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Comment on attachment 8344819 [details] [diff] [review]
> mochitest v1.patch
> 
> Clipboard access is not necessarily synchronous.  We have a helper in our
> testing framework for this: SimpleTest.waitForClipboard.
> 
> I'm not sure what this is trying to test.  Why is this not covered by our
> existing tests?

It is to test the implementation of nsIClipboard.idl. More specifically, test
these methods:
   nsIClipboard.setData,
   nsIClipboard.getData,
   nsIClipboard.emptyClipboard.

The patch copied some code from
http://mxr.mozilla.org/mozilla-central/source/widget/tests/test_bug462106_perwindow.xul

Is there an exiting tests covering this?

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Awesome! Thanks Yuan.

Comment 6

[(no longer active)]

I think <http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_copypaste.html?force=1> covers this (we also have other tests which exercise the clipboard code, please grep for waitForClipboard).

Comment 7

[(no longer active)]

Actually given the work done in bug 946646, it might make sense to have this test anyway.

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

So I'm testing this and on OSX |emptyClipboard| does not clear the clipboard, as also seen in bug 666254. And that's why this test fails. So yeah, we can only run this against Firefox OS, or abandon it altogether.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

And I can't empty the clipboard on OSX by putting an empty string because of bug 956676.

Comment 10

[(no longer active)]

See bug 666254 comment 5.  This is sort of expected on OSX, given the current implementation.  I think the best path forward is to skip that part of the test on OSX.

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

New try with test disabled on OSX:  https://tbpl.mozilla.org/?tree=Try&rev=805e3b98673c

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: clipboard_mochi2.patch

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Did this ever get review from the correct peer?

Comment 14

[Jan Jongboom [:janjongboom] (inactive)]

Hmm, I don't know. ni'ing @yxl

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

No :-(

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8358393 [details] [diff] [review]
clipboard_mochi2.patch

So why did it have r=fabrice? :p

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8358393 [details] [diff] [review]
clipboard_mochi2.patch

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

Moving over to Ehsan that know clipboard code better!

Comment 18

[(no longer active)]

Comment on attachment 8358393 [details] [diff] [review]
clipboard_mochi2.patch

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

This looks mostly good, but I'd like to look at another version of the patch which addresses the comments below.  Thanks!

::: widget/tests/chrome.ini
@@ +28,5 @@
>  [test_bug760802.xul]
>  
> +[test_clipboard.xul]
> +# Bug https://bugzilla.mozilla.org/show_bug.cgi?id=666254#c5
> +skip-if = toolkit == "cocoa"

Hmm, I meant only skipping the emptyClipboard part of the test, not all of it.  You can do that by detecting whether the test is being run on Mac like this: <http://dxr.mozilla.org/mozilla-central/source/layout/inspector/tests/chrome/test_bug467669.xul#25> and then just skip over the emptyClipboard part if kIsMac is true.

::: widget/tests/test_clipboard.xul
@@ +60,5 @@
> +
> +    let clipboard = Cc['@mozilla.org/widget/clipboard;1']
> +               .getService(Ci.nsIClipboard);
> +    ok(clipboard, 'Should have clipboard servive.');
> +    if (!clipboard) {

This block and the similar ones here are not needed.  Basically in mochitests we assume that the entire test passes, and if some test such as the one on the previous line fails and then doing accessing the clipboard variable later on fails because it's undefined, that's fine, since that will throw and the test framework handles unhandled exceptions as test failures.

@@ +75,5 @@
> +      SimpleTest.finish();
> +      return;
> +    }
> +    helper.copyString(data, document);
> +    is(data, paste(clipboard), 'Data was successfully copied.');

Nit: please pass the value to be tested as the first value and the expected value as the second one.

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

Xulei, the patch here is still yours. You want to re-take the bug or should I do stuff with it?

Comment 20

[Yuan Xulei [:yxl] (Leave from May 18)]

Please take it and complete the patch if you have time :-)

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

Changed the patch and have it running in try, let's see what happens. https://tbpl.mozilla.org/?tree=Try&rev=f0d6de4dbabb

Comment 22

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v3

Forgot to include the updated patch last week

Comment 23

[(no longer active)]

Comment on attachment 8380573 [details] [diff] [review]
Patch v3

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

r- mostly for the kIsMac issue.  But this is really close now!

::: widget/tests/test_clipboard.xul
@@ +56,5 @@
> +    return str;
> +  }
> +
> +  function initAndRunTests() {
> +    SimpleTest.waitForExplicitFinish();

Hmm, this is a synchronous test, you don't need this.

@@ +59,5 @@
> +  function initAndRunTests() {
> +    SimpleTest.waitForExplicitFinish();
> +
> +    let clipboard = Cc['@mozilla.org/widget/clipboard;1']
> +               .getService(Ci.nsIClipboard);

Nit: please fix the indentation.

@@ +69,5 @@
> +    helper.copyString(data, document);
> +    is(data, paste(clipboard), 'Data was successfully copied.');
> +
> +    // Test emptyClipboard, disabled for OSX because bug 666254
> +    if (!kIsMac) {

Hmm, you haven't defined kIsMac anywhere, you need that.  See the test failures on the try push.

@@ +73,5 @@
> +    if (!kIsMac) {
> +      clipboard.emptyClipboard(Ci.nsIClipboard.kGlobalClipboard);
> +      is('', paste(clipboard), 'Data was successfully cleared.');
> +    }
> +    SimpleTest.finish();

Please get rid of this as well.

Comment 24

[Jan Jongboom [:janjongboom] (inactive)]

Try is green with kIsMac added. https://tbpl.mozilla.org/?tree=Try&rev=4fb1c3814418

Let me upload the updated patch and let's all be happy. :-)

Comment 25

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: 948065_v4.patch

Patch with the nits from Ehsan addressed. Try @ https://tbpl.mozilla.org/?tree=Try&rev=5e7716f50c1a

Comment 26

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8386028 [details] [diff] [review]
948065_v4.patch

Try is green except for some intermittents. Can you do final check, sorry for letting this slip so long.

Comment 27

[(no longer active)]

Comment on attachment 8386028 [details] [diff] [review]
948065_v4.patch

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

Looks good, thanks!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e76bcb9087

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b8e76bcb9087