dart-archive / angular.dart

Legacy source repository. See github.com/dart-lang/angular
https://webdev.dartlang.org/angular/
1.24k stars 248 forks source link

lib/core/exception_handler.dart uses wrong log level for exceptions #1706

Open goderbauer opened 9 years ago

goderbauer commented 9 years ago

lib/core/exception_handler.dart uses "print" to log an exception. This will result in a log entry with log level "INFO" in the browser's console. That's not really appropriate for an exception, which should really have log level "ERROR".

This is a problem, because we are checking at the end of each webdriver test, if there are any exceptions present in the browser. For this, we go through the log entries in the console and if we find one that has a log level of ERROR, we mark the test as failed. Unfortunately, due to the bug described above we miss some exceptions because they don't have the appropriate log level. Therefore, some tests are marked as passed even though they really failed and there is no good way of detecting this if exceptions are not logged with the appropriate log level.

tbosch commented 9 years ago

Note that simply changing the ExceptionHandler to import dart:html and do a window.console.log does not work on first try, probably because of transformer issues...

rkirov commented 9 years ago

Given angular.dart is in maintenance mode, I am reluctant to change the default behavior of our exceptionHandler.

Note however that it is trivial to rebind your own exception handler in the tests to perform whatever task makes most sense in the context.

@Injectable()
class RethrowingExceptionHandler {
  call(dynamic error, dynamic stack, [String reason = '']) {
    throw error;
  }
}

main() {
  applicationFactory().addModule(
      new Module()
      ..bind(MyCmp)
      ..bind(ExceptionHandler, toImplementation: RethrowingExceptionHandler)
    ).run();
}
goderbauer commented 9 years ago

I am aware of that and that's what we are already doing as a workaround for now. However, we have many people writing many little benchmark apps and it is easy to forget that one bind line. If worse comes to worse, you will never know that you forgot the line because your tests never fail due to this bug.

I feel like the default implementation should at least log the exception correctly as an error.