Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

testing: exceptions silently ignored in EventBus subscribtions #443

Closed
jpommerening opened this issue Mar 31, 2017 · 5 comments
Closed

testing: exceptions silently ignored in EventBus subscribtions #443

jpommerening opened this issue Mar 31, 2017 · 5 comments

Comments

@jpommerening
Copy link
Member

I was testing the following code:

debugEventBus.subscribe( `didLoad.page.${instance}`, ( { [ type ]: ref } ) => {
   console.info( ... ); // added this line because was not 100% sure this was called w/ karma
   log.debug( ... ); // added this line earlier to get some feedback in my browser
   pages.push( ref );
   console.info( ... );  // had to add this to notice that "log" was undefined
} );

So, sure enough, I forgot to pass the log instance in my test setup. However, shouldn't we log something when subscribers throw?

@x1B
Copy link
Member

x1B commented Apr 5, 2017

Since eventBus.create accepts an error-handler function as the last argument, the mock event bus should probably always configure a "loud" error handler.

Regarding general log-behaviour in tests I am on the fence:

  • On on hand, I like to be able to test behavior that causes log.error on purpose without being confused by red/yellow messages in terminal or browser console.
  • On the other hand, "mute by default" makes diagnosing actual problems much harder.

Possible solution:

  • allow to setup mock log "expectations" that will then be silently dropped, while all other logging goes through.
  • users could also be able to require no unexpected logging, turning any additional log message (above the specified level) into a test failure.

@jpommerening
Copy link
Member Author

@x1B so the mock event bus currently does log the error, it's just not appearing because of the test infrastructure?
I'm not having an issue with logging in general; couldn't we just re-throw the error (or a new one with the message constructed by the event bus) when testing?
I don't think there is a legitimate use case for testing throwing subscribers. IMO subscriptions that throw should always cause tests to fail.

@x1B
Copy link
Member

x1B commented Apr 6, 2017

@jpommerening agreed, let's make it so (tm)

@x1B x1B self-assigned this Apr 6, 2017
@x1B x1B added this to the v2.0.0 milestone Apr 6, 2017
@x1B
Copy link
Member

x1B commented Apr 7, 2017

Just for completeness: this is not a problem in widget tests (or we would've noticed this far sooner), because laxar-mocks passes a jasmine-aware error handler to the mock event bus:

https://github.com/LaxarJS/laxar-mocks/blob/fa85b6b/laxar-mocks.js#L238

@x1B
Copy link
Member

x1B commented Apr 7, 2017

Implemented on master (v2.0.0).

@x1B x1B closed this as completed Apr 7, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants