search

undercover-el / undercover.el

A test coverage library for Emacs
MIT License
86 stars 14 forks source link

Improve public API #67

Open CyberShadow opened 3 years ago

CyberShadow commented 3 years ago

Attempts to fix #66.

CyberShadow commented 3 years ago

@doublep I think this should now include (some form of) solutions for all the items discussed in #66, could you have a look and see if this is enough for Eldev's Undercover integration?

doublep commented 3 years ago

It doesn't look like :files are actually activated. If I simply eval sth. like (undercover (:files "lol.el")), I get a complaint about unsupported option.

CyberShadow commented 3 years ago

Right, fixed.

doublep commented 3 years ago

report-on-kill is completely broken.

CyberShadow commented 3 years ago

How is it broken?

Are you trying to use it with an expression? I think I may need to tweak the macro for that to work...

doublep commented 3 years ago

Evaluated before being set. (unless undercover--report-on-kill should be (when ..., maybe something else. Just test yourself.

CyberShadow commented 3 years ago

Well spotted, thanks!

doublep commented 3 years ago

When disabling report-on-kill and calling undercover-safe-report manually, I have no idea if it has information or not (because e.g. it got disabled on a non-CI machine). So, I get an error message for nothing.

CyberShadow commented 3 years ago

Hmm, the semi-relying on defaults thing again.

I guess there's no harm in making undercover--coverage-enabled-p public, might as well. Could be useful for other scenarios such as working around bugs.

doublep commented 3 years ago

Hmm, the semi-relying on defaults thing again.

I don't want to duplicate the logic of "am I on CI or not?". It is easy to enable local report generation, but always doing it is annoyingly slow. Always disabling is also not a good idea as then you lose the "do the right thing without configuration on CI", which is 99% of usecases. So instead I use "default to CI only, unless explicitly told otherwise on command line" logic.

Please also make undercover--message public so that it can be advised. Alternatively, pipe all message through a user-provided function (see e.g. iter2-tracing-function here: it provides the default tracing, but can easily be replaced with something else too). Usecase: Eldev highlights errors/warning/normal output/debug output differently and sends to different channels, so I need to know the verbosity level associated with the message.

CyberShadow commented 3 years ago

Please also make undercover--message public so that it can be advised.

That's a really bad reason to make a function public.

CyberShadow commented 3 years ago

Putting undercover--message behind an indirect call means that in the future I can't make undercover--message a macro thatevaluates its arguments lazily depending on the log level, in case future versions will need to maybe log things that have a non-trivial cost to evaluate.

Should I keep the (when (<= level undercover--verbosity) check inside undercover--message? Implementers of undercover-message-function wouldn't be able to control whether the function is being called or not, aside from pre-emptively setting the verbosity level very high.

CyberShadow commented 3 years ago

Always disabling is also not a good idea as then you lose the "do the right thing without configuration on CI", which is 99% of usecases.

Yeah, that's not the problem and a good thing IMO. The tricky part is how to design everything to allow integrations such as Eldev, while not breaking the ability to improve what "do the right thing" means in future versions and new circumstances.

doublep commented 3 years ago

Sorry, I forgot about this PR.

Should I keep the (when (<= level undercover--verbosity) check inside undercover--message? Implementers of undercover-message-function wouldn't be able to control whether the function is being called or not, aside from pre-emptively setting the verbosity level very high.

Doesn't sound terribly important to me, but maybe you could abstract this away the same way as you have done with undercover-message-function. The default implementation would then use undercover--verbosity, but could be replaced. As of now, Eldev (the stash I have privately) does set verbosity to very high in order to filter later.

Yeah, that's not the problem and a good thing IMO.

Doesn't sound like a good thing to me. Essentially, "run by default only on a CI server" is a feature of undercover that I don't want to lose when it is run from Eldev. It is way too useful. If you want Eldev or whatever other higher-level wrapper to make this explicitly, expose undercover--under-ci-p publicly. But as it stands, it does not look really necessary, as undercover-enabled-p is now public.