Closed Bug 948065 Opened 6 years ago Closed 6 years ago

Mochitest for nsIClipboard

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
feature-b2g 2.0

People

(Reporter: xyuan, Assigned: janjongboom)

References

Details

Attachments

(1 file, 3 obsolete files)

Add mochitest to test the basic functions of nsIClipboard.
Blocks: 946646
No longer blocks: 938040
Attached patch mochitest v1.patch (obsolete) — Splinter Review
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?
(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?
Flags: needinfo?(ehsan)
Awesome! Thanks Yuan.
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).
Flags: needinfo?(ehsan)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
Actually given the work done in bug 946646, it might make sense to have this test anyway.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Assignee: xyuan → janjongboom
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.
Flags: needinfo?(ehsan)
And I can't empty the clipboard on OSX by putting an empty string because of bug 956676.
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.
Flags: needinfo?(ehsan)
Attached patch clipboard_mochi2.patch (obsolete) — Splinter Review
Attachment #8344819 - Attachment is obsolete: true
Attachment #8358393 - Flags: review+
Did this ever get review from the correct peer?
Keywords: checkin-needed
Hmm, I don't know. ni'ing @yxl
Flags: needinfo?(xyuan)
No :-(
Flags: needinfo?(xyuan)
Comment on attachment 8358393 [details] [diff] [review]
clipboard_mochi2.patch

So why did it have r=fabrice? :p
Attachment #8358393 - Flags: review+ → review?(fabrice)
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!
Attachment #8358393 - Flags: review?(fabrice) → review?(ehsan)
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.
Attachment #8358393 - Flags: review?(ehsan) → review-
Xulei, the patch here is still yours. You want to re-take the bug or should I do stuff with it?
Flags: needinfo?(xyuan)
Please take it and complete the patch if you have time :-)
Flags: needinfo?(xyuan)
Changed the patch and have it running in try, let's see what happens. https://tbpl.mozilla.org/?tree=Try&rev=f0d6de4dbabb
Attached patch Patch v3 (obsolete) — Splinter Review
Forgot to include the updated patch last week
Attachment #8358393 - Attachment is obsolete: true
Attachment #8380573 - Flags: review?(ehsan)
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.
Attachment #8380573 - Flags: review?(ehsan) → review-
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. :-)
Attached patch 948065_v4.patchSplinter Review
Patch with the nits from Ehsan addressed. Try @ https://tbpl.mozilla.org/?tree=Try&rev=5e7716f50c1a
Attachment #8380573 - Attachment is obsolete: true
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.
Attachment #8386028 - Flags: review?(ehsan)
Comment on attachment 8386028 [details] [diff] [review]
948065_v4.patch

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

Looks good, thanks!
Attachment #8386028 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/b8e76bcb9087
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.