Undercover causes weird errors in Emacs <26 when enabled

[DarwinAwardWinner]

So, I am trying to enable coverage reports using undercover for my project, amx. However, enabling undercover causes a bunch of my tests to fail with weird errors, as you can see here: https://travis-ci.org/DarwinAwardWinner/amx/builds/650744876

For comparison, here's the successful run right before undercover was enabled: https://travis-ci.org/DarwinAwardWinner/amx/builds/650732841

The errors all seem to be error: (error "Bad `using' clause"). Grepping the Emacs codebase seems to imply that this error is thrown from inside the bowels of the cl-loop macro, but I have no idea why enabling undercover would trigger this error in code that normally runs just fine.

[DarwinAwardWinner]

And I forgot to mention, Emacs 26 seems to run just fine. This error only occurs on 25 and lower. (Note: the build failure on emacs-snapshot is unrelated to this issue.)

[doublep]

Must be because in Emacs 25.3 source code there is no special handling of using in cl-loop's (declare (debug ...)) declaration. undercover uses Edebug-like instrumentation, so it needs a correct declaration. (I don't know how to use cl-loop in general and if using works on pre-26 Emacs at all.)

[DarwinAwardWinner]

Well, using must work in earlier Emacsen, because that code runs on everything from 24.4 onward: https://travis-ci.org/DarwinAwardWinner/amx/builds/650904986.

[doublep]

Then it rather looks like a bug in Emacs. I.e. it supplies a wrong debug declaration in pre-26 versions.

[DarwinAwardWinner]

Yeah, using git blame, it looks like it was fixed before 26.1:

• Bug: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24750
• Fix: emacs-mirror/emacs@fc01cfc

[CyberShadow]

Thanks for looking into it; is there still anything that needs to be done on undercover.el's side?

[DarwinAwardWinner]

I guess undercover.el could supply the correct debug spec. This should be pretty safe to do. I doubt that fixing an incorrect debug spec would ever break any existing code, and even if it does it will only break it in tests, so it won't mess up anyone's daily work.

[CyberShadow]

OK, how about this patch, then?

CyberShadow@issue-54

[DarwinAwardWinner]

Seems reasonable.

[CyberShadow]

@DarwinAwardWinner Could you please check if the patch fixes the error you were seeing (if you haven't done so yet)?

[DarwinAwardWinner]

Hmm, the current master of undercover seems to be crashing on all Emacs versions in my test suite, which is weird because it wasn't doing that as recently as last week. So I guess I'll need to figure that out before I can test whether this patch fixes things.

https://github.com/DarwinAwardWinner/amx/actions/runs/506341941

[DarwinAwardWinner]

Oh wait, part of the issue it's that it's using melpa-stable. I need to debug my test suite.

[CyberShadow]

If you can get it to print a stack trace for that (wrong-type-argument stringp nil), it might help us understand what's going on.

[DarwinAwardWinner]

Weird, I'm already running eldev with --debug and --trace, so it should print the stack trace. And unfortunately it only seems to be happening in CI. I can't reproduce it locally with UNDERCOVER_FORCE=true eldev -s -dtT test

[CyberShadow]

I can reproduce it with GITHUB_ACTIONS=true.

[CyberShadow]

Hmm, it looks like eldev attempts to integrate deeply with Undercover's internals, going as far as advising private functions. That might have something to do with it, as I moved a lot of things around under the hood in v0.8.0.

[DarwinAwardWinner]

Sure enough, GITHUB_ACTIONS=true breaks it on my machine as well.

[DarwinAwardWinner]

Ok, so it looks like I will need to wait for an Eldev update before I can use undercover again.

@doublep Pinging you to make you aware.

[CyberShadow]

The error originates from this line:

https://github.com/doublep/eldev/blob/ef6ea342a2dee7d6e745bc48d4d0f50d4552d537/eldev-plugins.el#L336

In v0.8.0, I allowed every :report-format type to have its own default report file path. As part of this, I changed the default value of undercover--report-file-path to nil, so that it is filled in by the report type specific processor with its default.

undercover--report-file-path is still nil when that Eldev line executes, which gets passed on to file-exists-p, hence the error.

For the long term, Undercover's public API needs to be expanded to accommodate such use cases directly, without needing to use advice or accessing private variables. That would help not only Eldev in terms of long-term stability, but any other package that wants to build on top of Undercover with similar goals / intentions.

[doublep]

Yeah, I had to go into internals because publicly exposed stuff wasn't enough to achieve what I wanted. I partially rewrote integration code, but it of course still goes into internals, as there is no better way.

If you feel like doing something about it, I could provide the list of what Eldev needs.

[CyberShadow]

If you feel like doing something about it, I could provide the list of what Eldev needs.

Yes, please.