The default bug view has changed. See this FAQ.

Unit tests for Workspaces

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Kevin Dangoor, Assigned: msucan)

Tracking

unspecified
Firefox 6
Points:
---
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

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

Comment 1

6 years ago
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)
Attachment #523405 - Flags: feedback?(rcampbell)
(Assignee)

Updated

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

Comment 2

6 years ago
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.
Attachment #523405 - Attachment is obsolete: true
Attachment #523405 - Flags: feedback?(rcampbell)
Attachment #524217 - Flags: feedback?(rcampbell)
(Assignee)

Updated

6 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

6 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

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

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

Updated

6 years ago
Blocks: 646070
global-nit: https://www.mozilla.org/MPL/boilerplate-1.1/pd-c :D
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

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

Updated

6 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

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

Updated

6 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

6 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: 6 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
You need to log in before you can comment on or make changes to this bug.