Skip to content

Conversation

sotojuan
Copy link
Contributor

Fixes #775.

WIP because I need to add some tests + need more review especially for testing. Would it be better to remove the time from all the modified tests if I have some specific tests that do test time on watch?

@novemberborn
Copy link
Member

Would it be better to remove the time from all the modified tests if I have some specific tests that do test time on watch?

Yea. Having to add the time in all those tests should've been a clue that time was being output too much.


Passing the boolean is a bit awkward, but I think it's OK. Ideally the logger understands test runs, so we don't have to interact with it as precisely as we do in the watcher.

@sotojuan
Copy link
Contributor Author

Fixed tests.

@novemberborn
Copy link
Member

Can probably move these two lines into the new test now.

LGTMO!

@jamestalmage
Copy link
Contributor

jamestalmage commented May 18, 2016

I think we should pass an options object to the reporter constructor with static details/config like isWatching.

It definitely shouldn't be a boolean param. {watching: true}

@sotojuan
Copy link
Contributor Author

Good idea—just pushed some changes to make that work.

cli.js Outdated
reporter = verboseReporter();
} else {
reporter = miniReporter();
reporter = miniReporter({isWatching: cli.flags.watch});
Copy link
Contributor

Choose a reason for hiding this comment

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

isWatching => watching

@jamestalmage
Copy link
Contributor

Just so we're all on the same page about reporter constructor options. IMO, I think the reporters should avoid maintaining any mutable state, especially as it relates to the test run status. We are violating that rule in a couple spots now (my fault, I was supposed to follow up after merging the PR that introduced RunStatus).

I do think it's fine to pass immutable config values that will never change during the test run (i.e. watching, basedir, pkgConf, debugging, etc). That should keep our reporters as state free and easily testable as possible, without necessitating passing around needless config data.

@sotojuan
Copy link
Contributor Author

Pushed the changes (and thanks for the explanation, makes sense).

@jamestalmage
Copy link
Contributor

@sotojuan

Move the two lines @novemberborn talked about here into the test that uses it.

Once you've done that (and tests pass), go ahead and merge.

@sotojuan sotojuan changed the title [WIP] Remove timestamp from non-watch test runs Remove timestamp from non-watch test runs May 20, 2016
@sotojuan sotojuan merged commit 7d0f420 into avajs:master May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants