Need a testsuite for the error console

RESOLVED WONTFIX

Status

Toolkit Graveyard
Error Console
RESOLVED WONTFIX
9 years ago
2 years ago

People

(Reporter: Natch, Unassigned)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

9 years ago
Is there any way to create an nsIScriptError aside from try{ ...some bad code...} catch(e){ ...use e... }?

Can't really do that much testing without first creating an error object to test with!
Status: NEW → ASSIGNED
(Reporter)

Comment 2

9 years ago
Created attachment 374989 [details] [diff] [review]
patch

Basically this is pure craziness :)

The attached patch is the only way to "reliably" get the tests to pass intermittently. The only other way I can think of is listen to DOMNodeInserted on the console or adding a notifyObservers of some sort (maybe test only? is that possible?).

I'll try DOMNodeInserted tomorrow and see if it's practical. Btw, this runs the tests about 4-5 times in arbitrary order to the nature of registerListener...
(Reporter)

Comment 3

9 years ago
Created attachment 375099 [details] [diff] [review]
much better

All tests pass now with a few code modifications I'll put in an updated patch to bug 489736 (besides the string comparison check, simple fix). There's still a lot involved in this test, not sure if it still is tree-worthy but there really is no other way of testing this, thoughts?
Attachment #374989 - Attachment is obsolete: true
(Reporter)

Comment 4

9 years ago
Shawn: given the complexity of this test, do you think it is still tree-worthy?

No matter how you cut the cake, you'll need a message observer and a mutation listener to successfully test the input/output of data from the console. Then you need a whole bunch of other variables to protect the order and state of the tests.

Just one example of what can go wrong (which the test has to guard against) is an error being thrown during this test (something which the bookmark code seems to enjoy doing). That could ruin all the assumptions about the state of the console, and is part of the reason the complexity was turned up a notch...
First, isn't the error console a toolkit thing?  This should really be a chrome test insides of toolkit/

Instead of making an error happen with code, would Components.utils.reportError not work?
(Reporter)

Comment 6

9 years ago
(In reply to comment #5)
> First, isn't the error console a toolkit thing?  This should really be a chrome
> test insides of toolkit/

It should, but I'm not very acquainted with the build code involved to do this, I'll give it a shot.
> Instead of making an error happen with code, would Components.utils.reportError
> not work?

It *might* work if I pass in a "pseudo-error" object, otherwise we can't create a real error object on our own, see comment 1. It would have to look something like:
{
errorMessage: "Test",
sourceName: "test://",
sourceLine: "test function()",
lineNumber: 5,
columnNumber: 5,
QueryInterface: function(aIID){return aIID.equals(Ci.nsIScriptError)}
}

I haven't tested this and I'm not sure what the side-effect of doing this would be, but if you think it's worth a try, sure.
(Reporter)

Comment 7

9 years ago
...and confirmed that doesn't work :(

If it's so bad to throw during the test like that, maybe we should just do Cu.reportError("Some Error") even if we can't test as much (syntax error gives source code and column number information, reportError only gives line, message, and file information).
(Reporter)

Comment 8

9 years ago
Created attachment 375124 [details] [diff] [review]
toolkit chrome test + lots of comments

Tests are ready for review.
Attachment #375099 - Attachment is obsolete: true
Attachment #375124 - Flags: review?(zeniko)

Comment 9

9 years ago
Comment on attachment 375124 [details] [diff] [review]
toolkit chrome test + lots of comments

>diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in
>+                 browser_errorConsole.js \

Part of an old patch?

>+ * This test ensures that the error console works! Added by Bug 490499.

I was of the impression that we first wanted to make sure that the current Error Console works as expected, so that we can just re-run the tests after your modifications for checking if it still works as it does now. Am I mistaken? Otherwise you could just as well append these tests to the patch over in bug 489736 (on which it depends, as I've found out once I tried to run it without your main patch).

>+        this.tests.push([0, this.console.console.children.length, "Console should clear on null message"]);

Any reason you don't just test right here (i.e. is(0, ...) instead of this.tests.push([0, ...]) )?
Attachment #375124 - Flags: review?(zeniko)
(Reporter)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 375124 [details] [diff] [review])
> >diff --git a/browser/base/content/test/Makefile.in b/browser/base/content/test/Makefile.in
> >+                 browser_errorConsole.js \
> 
> Part of an old patch?

yes, will remove.
> I was of the impression that we first wanted to make sure that the current
> Error Console works as expected, so that we can just re-run the tests after
> your modifications for checking if it still works as it does now. Am I
> mistaken?

That would be a lot more work for no apparent reason, these tests should just cover the features of the error console, both past and new, and the fact that they pass shows that 1) the error console works as expected, and 2) ensures that no future regressions will go unnoticed. Lots of these tests don't apply at all to the current error console and the others that do would require significant recoding to work.

> Otherwise you could just as well append these tests to the patch over
> in bug 489736 (on which it depends, as I've found out once I tried to run it
> without your main patch).

I wanted to keep this bug open and add more tests once the initial testsuite landed. I thought it would just add more clutter to the patch currently in bug 489736. If you want me to do combine them, by all means.

> >+        this.tests.push([0, this.console.console.children.length, "Console should clear on null message"]);
> 
> Any reason you don't just test right here (i.e. is(0, ...) instead of
> this.tests.push([0, ...]) )?
I don't remember why I did this, but I'll definitely change this.

Comment 11

9 years ago
(In reply to comment #10)
> That would be a lot more work for no apparent reason, these tests should just
> cover the features of the error console, both past and new, and the fact that
> they pass shows that 1) the error console works as expected, and 2) ensures
> that no future regressions will go unnoticed.

The reason would be to ensure that there's no unintended change in behavior, especially WRT code evaluation. However you're right that considering that it's not a mission critical feature, it might not be worth the additional effort.

> I wanted to keep this bug open and add more tests once the initial testsuite
> landed.

Right, adjusting the dependency tree accordingly...

So I'd prefer to first review the code changes before coming back to the tests.
No longer blocks: 489736
Depends on: 489736
(Reporter)

Updated

9 years ago
Blocks: 305206
(Reporter)

Updated

9 years ago
No longer blocks: 305206
Depends on: 305206
(Reporter)

Comment 12

9 years ago
Created attachment 394113 [details] [diff] [review]
updated tests

Updated the tests to be a lot more sane, didn't fully check them yet. No need for the funky error throwing anymore!
Attachment #375124 - Attachment is obsolete: true
(Reporter)

Comment 13

9 years ago
Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

8 years ago
Assignee: nobody → rcampbell
Depends on: 593540
No longer depends on: 305206, 489736
I incorporated bits of your code from this patch as a template for bug 593540. Should be good for starters.
Assignee: rcampbell → nobody
The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)
Status: RESOLVED → REOPENED
Component: Error Console → Developer Tools: Console
Product: Toolkit → Firefox
Resolution: WONTFIX → ---
The DevTools console has a test suite, and error console is gone now.
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → WONTFIX
Component: Developer Tools: Console → Error Console
Product: Firefox → Toolkit
(Assignee)

Updated

2 years ago
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.