wheresrhys / fetch-mock

Mock http requests made using fetch
http://www.wheresrhys.co.uk/fetch-mock/
MIT License
1.26k stars 177 forks source link

Please no pro-tips #108

Closed rsheldiii closed 8 years ago

rsheldiii commented 8 years ago

I get a pro-tip at the end of my tests now, and I don't want a protip at the end of my tests. Can there at least be a configuration value to turn it off?

Thanks

MarkASwanson commented 8 years ago

+1 As workaround solution for now, you can delete this line:

// node_modules/fetch-mock/src/server.js
// (and / or) node_modules/fetch-mock/es5/server.js
require('protips')(path.join(__dirname, '../', 'PROTIPS.md'));
jessicaschissato-zz commented 8 years ago

+1

wheresrhys commented 8 years ago

I'm surprised at the negative reaction. I will accept a PR to configure it off, but am also interested why you see it as so bad. Try to understand it from my point of view; A lot of users ask the same questions over and over again, and don't read the docs; protips is an attempt to address that. You may get a slightly irritating few extra lines in your console output... but is that really so bad?? I use fetch-mock daily, and it's really quite easy to ignore the protips, just as it's easy to ignore the majority of console output from most things which is mostly not useful noise

MarkASwanson commented 8 years ago

It's definitely a good idea and I learned a few really great tips. It is nicely formatted as well and pleasant to look at.

That being said, the chunk of text at the end makes less of my test output available to see, and it's more likely that I'll have to scroll up depending on what I'm looking for. Also, I've seen them all a bunch of times now and don't get any additional benefit from seeing them again.

I think it's nice to have them there by default, because as you say many people won't even read the docs. It would be doubly nice to have a little note at the bottom of each tip mentioning how one might turn them off once you've seen them all.

In any case, I appreciate all your hard work on this project and your continuing (creative) effort to make things easier for new users!

Lokua commented 8 years ago

I agree with @MarkASwanson. It is a very cool feature, but becomes distracting.

jamwaffles commented 8 years ago

@wheresrhys I also find the pro tips distracting and adding unnecessary noise, but you make a good point from a maintainer's point of view. As a useful compromise, could all these pro tips not go into a quickstart guide or some kind of cheatsheet type thing? Users can find that document or be directed to it instead of you and others regurgitating answers to the same questions.

At the least, I can handle pro tips being turned on by default, but please add a configuration flag so people who don't want them can turn them off.

c0 commented 8 years ago

Is it possible to only display them when a test fails? That would be more beneficial while troubleshooting.

kennethlynne commented 8 years ago

Good idea to have protips, but should be dead simple to disable. I do not want protips on my CI for instance.

wheresrhys commented 8 years ago

I won't have time for at least a week to work on this. PR to fetch-mock (and possibly to protips too to enable disabling) welcome

pdehaan commented 8 years ago

+1 for no protips by default (at least in CI environments).

This may be a quick-n-easy hack and opt-out if we tweak ./src/server.js:

if (!(process.env.CI || process.env.CONTINUOUS_INTEGRATION || process.env.NO_PROTIPS)) {
  require('protips')(path.join(__dirname, '../', 'PROTIPS.md'));
}

It looks like Travis-CI sets a CI and CONTINUOUS_INTREGRATION env variable to true (per https://docs.travis-ci.com/user/environment-variables/#Default-Environment-Variables) And Circle-CI sets a CI and CIRCLECI env variable to true (per https://circleci.com/docs/environment-variables/)

Failing those, you can opt-out by specifying a NO_PROTIPS env variable, for example:

# no output
$ CI=true node src/server   

# no output
$ CONTINUOUS_INTEGRATION=true node src/server    

# no output
$ NO_PROTIPS=true node src/server    

# Protip displayed, default behavior.
$ node src/server    

/* * * * * * * * * * * * * * * * * * * * * * * */
Pro tip for fetch-mock:
To delay responses you can wrap the response in a `Promise`

fetchMock.mock('^http://example.com', new Promise(res => setTimeout(res, 200)).then(() => 200));

/* * * * * * * * * * * * * * * * * * * * * * * */

So if my theories are correct, this should automatically disable the PROTIPS on [at least] Travis-CI and Circle-CI environments and users can always opt-out by shotgunning NO_PROTIPS=true on the command line or globally as an environment variable.

combatpoodle commented 8 years ago

Throwing rando stuff into peoples' unit tests? No fly zone...

joshhunt commented 8 years ago

Just to reiterate - while I can understand the what lead to this (and I get that you're only trying to help), it just amounts to extra distracting spam in CI and our consoles and it doesn't set a great precedent for future libraries. What happens when the other utils we use in our tests see this and start doing the same?

Also consider that you're discouraging other people from using this library within their own that they're wishing to publish.

screen shot 2016-07-11 at 7 32 55 pm

To be completely honest, when we run our tests we're not interested in learning more about your library - we just want to see the result of our tests. It's quite presumptive to assume otherwise and inject yourself at the end of our scripts.

We'll be pinning to v4.5.4 until these pro tips are disabled by default.

wheresrhys commented 8 years ago

@pdehaan 's PR is now merged (thanks for the submission), so it's possible to opt out. I'm currently trying to think of ways to keep it in but to output nothing during normal operation, and will probably remove it entirely unless I can think of a good way to do this.

Ah well, it was an interesting experiment at least. Maybe somebody has a view on this too https://github.com/wheresrhys/fetch-mock/issues/80

pdehaan commented 8 years ago

output nothing during normal operation

I guess it depends on what you consider "normal operation". End users locally versus continuous integration. I disabled in CI environments since adding noise there isn't useful (to us). I couldn't think of a good way to let end users opt in/out of it (apart from adding that extra env variable which they can do per-run or set globally on their machine).