490499 - Need a testsuite for the error console

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: Error Console, defect)

Comment 1

[Nochum Sossonko [:Natch]]

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!

Comment 2

[Nochum Sossonko [:Natch]]

Attachment: 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...

Comment 3

[Nochum Sossonko [:Natch]]

Attachment: 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?

Comment 4

[Nochum Sossonko [:Natch]]

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...

Comment 5

[Shawn Wilsher :sdwilsh]

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?

Comment 6

[Nochum Sossonko [:Natch]]

(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.

Comment 7

[Nochum Sossonko [:Natch]]

...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).

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: toolkit chrome test + lots of comments

Tests are ready for review.

Comment 9

[Simon Bünzli]

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, ...]) )?

Comment 10

[Nochum Sossonko [:Natch]]

(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

[Simon Bünzli]

(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.

Comment 12

[Nochum Sossonko [:Natch]]

Attachment: 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!

Comment 13

[Nochum Sossonko [:Natch]]

Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...

Comment 14

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

I incorporated bits of your code from this patch as a template for bug 593540. Should be good for starters.

Comment 15

[Brian Grinstead [:bgrins]]

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.

Comment 16

[Zack Weinberg (:zwol)]

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.)

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The DevTools console has a test suite, and error console is gone now.