Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop unnecessary console output from testing #108

Closed
wants to merge 2 commits into from
Closed

Drop unnecessary console output from testing #108

wants to merge 2 commits into from

Conversation

earldouglas
Copy link
Contributor

These console outputs were useful at one point, during the development of certain tests, but now are just noising up the test output.

Ideally, the passing/failing of tests should tell us what we need to know about our test results, so nothing from the console should be needed, except for ad-hoc debugging of tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) when pulling 1d89977 on earldouglas:quiet-tests into ac8ec68 on wikimedia:master.

return restbase({
logging: {
name: 'restbase-tests',
level: offline ? 'fatal' : 'warn', // hide warnings during offline tests
level: 'fatal'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we default to 'warn' level?

@earldouglas
Copy link
Contributor Author

Chatted with @gwicke , and we realized a better solution is to use a monoidal log, rather than the console, that we can check at the end of execution.

@earldouglas
Copy link
Contributor Author

I added a super crappy test logger that collects all log entries, which we can check at the end. It would be good to improve this by adding per-test checks that look at the log and verify that they introduced the expected entries, if any.

On the plus side, we're not left to visual inspection of the test output to catch unexpected log entries -- we can explicitly test for them instead.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.02%) when pulling f620e54 on earldouglas:quiet-tests into ac8ec68 on wikimedia:master.

@earldouglas
Copy link
Contributor Author

Oof, this hurts coverage,since it no longer exercies the real logger.

function collect(context) {
return function() {
var ctx = JSON.stringify(context);
if (/^warn/.test(arguments[0]) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: You could simplify this to /^(?:warn|error|fatal)/.test()

@earldouglas
Copy link
Contributor Author

Closing this in favor of an alternative in progress.

@earldouglas earldouglas closed this Jan 8, 2015
@earldouglas earldouglas deleted the quiet-tests branch January 8, 2015 18:55
@earldouglas earldouglas restored the quiet-tests branch January 8, 2015 18:55
@earldouglas earldouglas deleted the quiet-tests branch January 15, 2015 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants