Last Comment Bug 636725 - Unit tests for Workspaces
: Unit tests for Workspaces
Status: RESOLVED FIXED
[workspaces][patchclean:0420][fixed-i...
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Mihai Sucan [:msucan]
:
:
Mentors:
Depends on: 642176
Blocks: 646070
  Show dependency treegraph
 
Reported: 2011-02-25 07:48 PST by Kevin Dangoor
Modified: 2011-05-09 12:18 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
proposed patch (22.52 KB, patch)
2011-03-31 13:09 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
rebased patch (18.68 KB, patch)
2011-04-06 10:55 PDT, Mihai Sucan [:msucan]
rcampbell: review+
sdwilsh: review-
Details | Diff | Splinter Review
patch update 3 (13.63 KB, patch)
2011-04-15 13:30 PDT, Mihai Sucan [:msucan]
sdwilsh: review-
Details | Diff | Splinter Review
[checked-in][in-devtools] patch update 4 (18.69 KB, patch)
2011-04-20 01:30 PDT, Mihai Sucan [:msucan]
sdwilsh: review+
Details | Diff | Splinter Review

Description Kevin Dangoor 2011-02-25 07:48:50 PST
In graduating from a hack to a useful tool, Workspaces should grow a collection of unit tests.
Comment 1 Mihai Sucan [:msucan] 2011-03-31 13:09:06 PDT
Created attachment 523405 [details] [diff] [review]
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] 2011-04-06 10:55:38 PDT
Created attachment 524217 [details] [diff] [review]
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) 2011-04-08 11:29:34 PDT
Comment on attachment 524217 [details] [diff] [review]
rebased patch

these look good to me. no nits, no, complaints.
Comment 4 Mihai Sucan [:msucan] 2011-04-08 12:42:34 PDT
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] 2011-04-11 01:40:44 PDT
Pushed this to the try server. Results are looking fine:

http://tbpl.mozilla.org/?tree=MozillaTry&rev=c11860ebbc86
Comment 6 Shawn Wilsher :sdwilsh 2011-04-13 13:35:41 PDT
global-nit: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c :D
Comment 7 Shawn Wilsher :sdwilsh 2011-04-13 13:54:36 PDT
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] 2011-04-15 13:30:17 PDT
Created attachment 526358 [details] [diff] [review]
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 2011-04-18 14:12:51 PDT
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 2011-04-18 14:14:04 PDT
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] 2011-04-20 01:30:03 PDT
Created attachment 527214 [details] [diff] [review]
[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 2011-04-20 13:36:46 PDT
Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

r=sdwilsh
Comment 13 Rob Campbell [:rc] (:robcee) 2011-04-21 06:57:46 PDT
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) 2011-05-09 12:18:31 PDT
Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

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

Note You need to log in before you can comment on or make changes to this bug.