We really have to capture any error send to the error console and add those to the log and report of the file under test. Right now we don't get any output at all which means there could be broken tests in a test-run we aren't aware of. Given that higher severity it would be great to see a fix in Mozmill 1.5.2.

Should be fairly easy to implement.

Yes, this is exactly what I was talking about. Running with --debug will show the error console which will output any error, but these get cleared during test runs and should most definitely be logged as they appear somewhere for debugging post test-run.

This would be nice from a debugging point of view, but I don't think this really is stop-ship for 1.5.2. Why should it be stop-ship for 1.5.2? If we have manifests, and better communication between JS and python would you hold that release for this feature? I don't think so. I'm leaving as ? to discuss tomorrow, but I am leaning toward -. If it is easy once the JS<-->python changes are done, then maybe we can fit it in, but it is definitely last on the list at this point.

The main problem with the current handling is that javascript errors aren't caught. That will result whether in a passing nor failing test. It will simply not appear in the test results. With hundreds of tests you will not be able to identify such a broken test, as long as you do not run it with --debug and consistently watching the Error Console. We have had this situation a couple of times, and we feel that it would be important enough. If it will not fit into 1.5.2 we should make it happen for 1.5.3.

We'll see if we can fit it in.

Patch for 1.5.2, but as only frame.js is modified it will most likely be the same for master.

Andrew, is it possible to listen for all errors which get added? In most cases we had errors from xpcom components, which the current patch will not cover, right?

Yeah, I didn't really think of that. I'll upload a new patch that perhaps uses a regex. Henrik, do you know of a test that causes an xpcom error? It would help to see what the string output is, and I'm not quite sure how to generate one on my own.

(In reply to comment #8) > Henrik, do you know of a test that causes an xpcom error? It would help to see > what the string output is, and I'm not quite sure how to generate one on my > own. I have proposed nsILocalFile or nsIFilePicker to Andrew on IRC. At least for the latter specify an invalid flag. It should cause an error in the XPCOM component.

Or simply execute the following in the error console: Components.classes["@mozilla.org/filepicker;1"].createInstance(Components.interfaces.nsIFilePickr);

Uses a regex for added robustness to ensure we catch all errors that could potentially arise.

Comment on attachment 502020 [details] [diff] [review] Patch 1.1 - Using a regex This looks good. I wish there were some good way to filter the messages we are observing so that we don't have to perform the regex match on every single message, but I looked through the interfaces and I don't see one. My only beef here is with the regex - I think it should be case insensitive. That way it will match ERROR or error as well as Error. That ought to make it slightly more robust.

I modified the regex to be case insensitive. hotfix-1.5.2: https://github.com/mozautomation/mozmill/commit/8b238423ceda6e419c529b3154faab508e5afe89 master: https://github.com/mozautomation/mozmill/commit/508264f99df888c8b93e6af9510486d1866385af

As talked with Andrew on IRC we will reopen this bug because it doesn't catch all possible cases. One big situation it misses if a syntax error is located in a test or module. Right now we are simply returning with > INFO Passed: 0 > INFO Failed: 0 > INFO Skipped: 0 You will never know about those failures when you run the complete test-suite. This case was one of the reasons we wanted to have JS errors in the output/report.

(In reply to comment #14) > As talked with Andrew on IRC we will reopen this bug because it doesn't catch > all possible cases. One big situation it misses if a syntax error is located in > a test or module. This is really not enough to go on here to explicitly solve the problem. Can you give us examples of the different types of errors that this doesn't catch? One thing you may want to try Andrew is to take the aMessage that you get from your observer and attempt to QueryInterface it to an nsIScriptError interface. If that succeeds, then you can check if there is an Error flag set on it, and return that error message if you find one. If you do not see an error flag, or if the QI to nsIScriptError fails, then attempt the regex expression solution you have now. I still think this is only going to be a 95-99% solution. So, Henrik, we need explicit types of messages from you that you want captured. nsIScriptError: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/idl/nsIScriptError.idl#39

Not sure what's not clear by the last comment. Simply make a test syntactically incorrect by i.e. removing a bracket.

fwiw, this would capture any syntax errors and report them, it might even catch execution errors if my head is clear right now.

That works great except the fact that we still report it as: INFO Passed: 0 INFO Failed: 0 INFO Skipped: 0 We have to add failure frame for this test.

(In reply to comment #18) > That works great except the fact that we still report it as: > > INFO Passed: 0 > INFO Failed: 0 > INFO Skipped: 0 > > We have to add failure frame for this test. The way we report failures is by test function not by test file, so I'm not sure what we would report as failing.

1.5.2: https://github.com/mozautomation/mozmill/commit/0c83ddffbe8458e9c4efb88b6384787e8f2dcb5d master: https://github.com/mozautomation/mozmill/commit/3373cf37d61aa35b81e022ef347b7876148b86c4

As it stands right now we will not be notified of test syntax errors. Where does event.fail add the exception to? Will that ever end-up in the report? Andrew, for the differentiation between chrome and content you should probably have a look at Console2. It's separating those errors elegantly.

(In reply to comment #21) > As it stands right now we will not be notified of test syntax errors. Where > does event.fail add the exception to? Will that ever end-up in the report? What do you mean? When I do --show-all I see this in the output: ERROR | Test Failure: {"message": "missing ; before statement", "lineNumber": 41, "

(In reply to comment #21) > As it stands right now we will not be notified of test syntax errors. Where > does event.fail add the exception to? Will that ever end-up in the report? I'm pretty sure the problem is that by the time we report this failure the python side's endTestListener has already been triggered so any further reports will be ignored. I'm not sure how easy/possible this is to fix. > Andrew, for the differentiation between chrome and content you should probably > have a look at Console2. It's separating those errors elegantly. Do you mean the addon? As far as I know you have to use nsIConsoleService to receive error console messages and that only has one method to retrieve all types of messages. So I'm guessing either they use regexes like we are, or they wrote their own error console from scratch.

(In reply to comment #22) > What do you mean? When I do --show-all I see this in the output: > > ERROR | Test Failure: {"message": "missing ; before statement", "lineNumber": > 41, " You see the output but that's all. It will not appear in the report and when you run a bunch of tests locally it's easy to miss because the failure count doesn't get updated. (In reply to comment #23) > Do you mean the addon? As far as I know you have to use nsIConsoleService to > receive error console messages and that only has one method to retrieve all > types of messages. So I'm guessing either they use regexes like we are, or they > wrote their own error console from scratch. Yes, the extension. I just wanted to mention it if you want to have a look at it, and if we could take some code if possible.

The reason Heather's patch wasn't reporting was because the endTest event doesn't get fired when there is a syntax error in the test. Due to the scope, it is difficult for us to get a handle on the test that has the syntax error, so the best we can do is synthesize a test object and send that to the python endTest listener. The result is that a failure will get reported as expected, but the python reporter won't know which test caused it. This doesn't really matter because the stack trace still gets printed out from the events.fail() function. This patch also modifies the error console regex to only report errors with 'resource' or 'chrome' urls, thus ignoring content JS errors from random websites.

I had the case when one single error in the test has been triggered multiple failures of the test. Reason is probably because we also report to the error console, which we probably shouldn't do.

Comment on attachment 505176 [details] [diff] [review] Capture syntax errors and ignore content JS errors So I moved the ConsoleListener into the Runner object's constructor and it no longer goes out of scope when there is a syntax error. This means that it will catch all errors including: i) Syntax errors caught by Heather's events.fail() in the loadFile() (see her patch) ii) 'Normal errors' caught by the Runner.wrapper function. In certain cases the console listener provides information where Runner.wrapper cannot (i.e invalid contract id) iii) (Theoretical) errors that aren't caught by either of the first two methods (i.e non-mozmill chrome errors which may or may not be relevant) The problem now is that errors are getting output twice, once by either Runner.wrapper or Heather's fix, and once by the ConsoleListener. Unfortunately, the ConsoleListener is async to the rest of the test module running code which means it is not currently possible to only print one or the other. This leaves us with three options: 1) Write synchronization code between the ConsoleListener and the test reporting system (could get complicated fast) 2) Drop the ConsoleListener altogether and live without errors in category iii). 3) Print out each error message twice and live with it. In my opinion, option 2 is our best bet (at least for hotfix-1.5.2). We still catch syntax errors, which was the original intent of this bug, plus we aren't even sure if we ever will catch errors in category iii. Option 1 seems too complicated for a tomorrow release, but might be worth looking into for 2.0.

Option 2 sounds perfect. That way we will also see errors in the output pane of the extension. I don't think with the feature to log js errors we never have to report anything to the js console anymore, or am I wrong?

As discussed on irc, we decided that option 2 was the way to go. This patch still synthesizes a test object to send to the python endTest listener so we can report properly on syntax errors.

Comment on attachment 505257 [details] [diff] [review] Patch 2.0 - Remove console listener completely works great, thanks.

hotfix-1.5.2: https://github.com/mozautomation/mozmill/commit/54a4c6450b81c44e2c1fd96455acea06f361659f master: https://github.com/mozautomation/mozmill/commit/8d91eb9cfd1d58ac754fa52ef3e79a4d8f500a42

Beside the fact we have empty failure messages for invalid interfaces, this patch works great. Tested a couple of different situations and we always get helpful information back. Marking as verified fixed. Thanks!