Unit tests for Workspaces

RESOLVED FIXED in Firefox 6

Status

defect
RESOLVED FIXED
8 years ago
11 months ago

People

(Reporter: dangoor, Assigned: msucan)

Tracking

unspecified
Firefox 6
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [workspaces][patchclean:0420][fixed-in-devtools][merged-to-mozilla-central])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

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

Comment 1

8 years ago
Posted patch proposed patch (obsolete) — Splinter Review
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)
Attachment #523405 - Flags: feedback?(rcampbell)
Assignee

Updated

8 years ago
Status: NEW → ASSIGNED
Depends on: 642176
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [workspaces] → [workspaces][patchclean:0331]
Assignee

Comment 2

8 years ago
Posted patch rebased patch (obsolete) — Splinter Review
Rebased the patch on top of the new attachment 524216 [details] [diff] [review] - see bug 642176 comment 10.
Attachment #523405 - Attachment is obsolete: true
Attachment #523405 - Flags: feedback?(rcampbell)
Attachment #524217 - Flags: feedback?(rcampbell)
Assignee

Updated

8 years ago
Whiteboard: [workspaces][patchclean:0331] → [workspaces][patchclean:0406]
Comment on attachment 524217 [details] [diff] [review]
rebased patch

these look good to me. no nits, no, complaints.
Attachment #524217 - Flags: feedback?(rcampbell) → review+
Assignee

Comment 4

8 years ago
Comment on attachment 524217 [details] [diff] [review]
rebased patch

Thanks for your review+ Robert! Asking for final review from Shawn.
Attachment #524217 - Flags: review?(sdwilsh)
Assignee

Comment 5

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

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

Updated

8 years ago
Blocks: 646070
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
Attachment #524217 - Flags: review?(sdwilsh) → review-
Assignee

Comment 8

8 years ago
Posted patch patch update 3 (obsolete) — Splinter Review
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!
Attachment #524217 - Attachment is obsolete: true
Attachment #526358 - Flags: review?(sdwilsh)
Assignee

Updated

8 years ago
Whiteboard: [workspaces][patchclean:0406] → [workspaces][patchclean:0415]
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 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.
Attachment #526358 - Flags: review?(sdwilsh) → review-
Assignee

Comment 11

8 years ago
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!
Attachment #526358 - Attachment is obsolete: true
Attachment #527214 - Flags: review?(sdwilsh)
Assignee

Updated

8 years ago
Whiteboard: [workspaces][patchclean:0415] → [workspaces][patchclean:0420]
Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

r=sdwilsh
Attachment #527214 - Flags: review?(sdwilsh) → review+
Assignee

Updated

8 years ago
Whiteboard: [workspaces][patchclean:0420] → [workspaces][patchclean:0420][checkin][requires-dependencies]
Whiteboard: [workspaces][patchclean:0420][checkin][requires-dependencies] → [workspaces][patchclean:0420][fixed-in-devtools]
Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

http://hg.mozilla.org/projects/devtools/rev/61fc20a05e32
Attachment #527214 - Attachment description: patch update 4 → [in-devtools] patch update 4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [workspaces][patchclean:0420][fixed-in-devtools] → [workspaces][patchclean:0420][fixed-in-devtools][merged-to-mozilla-central]
Target Milestone: --- → Firefox 6
Comment on attachment 527214 [details] [diff] [review]
[checked-in][in-devtools] patch update 4

http://hg.mozilla.org/mozilla-central/rev/61fc20a05e32
Attachment #527214 - Attachment description: [in-devtools] patch update 4 → [checked-in][in-devtools] patch update 4

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.