636725 - Unit tests for Workspaces

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

In graduating from a hack to a useful tool, Workspaces should grow a collection of unit tests.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This includes several tests for the new Workspace code.

I have made some minor code adjustments for test purposes to the main Workspace code as well.

Please let me know if new tests need to be added, or any other changes I have to make. Thanks!

(please note that this patch requires the patch from bug 642176, applied in the devtools repo)

Comment 2

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch on top of the new attachment 524216 [details] [diff] [review] - see bug 642176 comment 10.

Comment 3

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 524217 [details] [diff] [review]
rebased patch

these look good to me. no nits, no, complaints.

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 524217 [details] [diff] [review]
rebased patch

Thanks for your review+ Robert! Asking for final review from Shawn.

Comment 5

[Mihai Sucan [:msucan]]

Pushed this to the try server. Results are looking fine:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=c11860ebbc86

Comment 6

[Shawn Wilsher :sdwilsh]

global-nit: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c :D

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 524217 [details] [diff] [review]
rebased patch

In general, these tests do a great job of testing the API, but what about testing the UI elements?  Both things are important.

>+++ b/browser/base/content/test/browser_workspace_contexts.js
>+let wsWindow;
This is a global, and I'd like it to be prefixed as such (and I'd prefer something a bit more descriptive such as gWorkspaceWindow).  A comment explaining what it is would be handy too, since I had to read a bunch of code to figure that out.

>+++ b/browser/base/content/test/browser_workspace_files.js
philikon and mak have made a strong case that we should be writing out tests like we'd write our code, which means doing async I/O here too.  The reason for this is that add-on authors look at our test code to see how to do things, and we don't want to encourage synchronous I/O.

This file already has to change a bunch since I've asked you to change the import/save methods on the API anyway.


I also don't see tests for these methods/behaviors, which we should have:
- executing only selected text in the various ways that can happen
- ui updating based on clipboard data (need to make sure you clear the clipboard before running this test)
- Workspace.deselect
- Workspace.selectRange
- Workspace.evalInSandbox in the case where it throws an error for the chrome and content contexts
- Workspace.inspect
- Workspace.exportToFile for both cases of aNoConfirmation

Comment 8

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated the unit tests.

Changes:

- rebased on top of the latest Workspace patch, attachment 526355 [details] [diff] [review] from bug 642176.
- switched to the PD license. :)
- renamed wsWindow to gWorkspaceWindow.
- switched to async file operations.
- added some more testing.

Thanks for your review Shawn!

Comment 9

[Shawn Wilsher :sdwilsh]

I still don't see tests for some of the things I mentioned in comment 7:
> In general, these tests do a great job of testing the API, but what about
> testing the UI elements?  Both things are important.
> - ui updating based on clipboard data (need to make sure you clear the
> clipboard before running this test)
> - Workspace.evalInSandbox in the case where it throws an error for the chrome
> and content contexts
> - Workspace.exportToFile for both cases of aNoConfirmation

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 526358 [details] [diff] [review]
patch update 3

>+++ b/browser/base/content/test/browser_workspace_contexts.js
This test should also test that you can access chrome things in a chrome context.

>+++ b/browser/base/content/test/browser_workspace_execute_print.js
>+  ws.print();
>+
>+  is(content.wrappedJSObject.foobarBug636725, "a",
>+     "print() worked for the selected range");
>+
>+  is(ws.textbox.value, "window.foobarBug636725\n" +
>+                       "/* a */\n" +
>+                       "= 'c';\n" +
>+                       "window.foobarBug636725 = 'b';",
>+     "print() shows evaluation result in the textbox");
You should be testing the the selection is set accordingly here too before you test deselect.

>+++ b/browser/base/content/test/browser_workspace_files.js
>+// The temporary file content.
>+let gFileContent = "hello.world('bug636725-" + Date.now() + "');";
What is Date.now() for?

>+  // Create a temporary file.
>+  gFile = Services.dirsvc.get("TmpD", Ci.nsIFile);
>+  gFile.append("fileForBug636725-" + Date.now() + ".tmp");
>+  gFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);
Use FileUtils.jsm:
gFile = FileUtils.getFile("TmpD", ["fileForBug636725.tmp"]);
gFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0666);
You don't need to to use a date here, create unique will make it unique for you.

>+  // Done!
>+  gFile = null;
Remove gFile here too please.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: [checked-in][in-devtools] patch update 4

Updated the patch to address the review comments. Thanks Shawn!

- rebased on top of the latest attachment 527213 [details] [diff] [review] from bug 642176.
- more selection tests.
- added test for the statusbar UI update when context is changed.
- added UI tests for the menuitems.
- added test for exportToFile() with confirmation.

Looking forward to your review!

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

r=sdwilsh

Comment 13

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

http://hg.mozilla.org/projects/devtools/rev/61fc20a05e32

Comment 14

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

http://hg.mozilla.org/mozilla-central/rev/61fc20a05e32