I think it would be better if test-helper.el did not provide a feature

[tarsius]

Today I have opened issues against 69 packages that come with a file test-helper.el that provides the feature test-helper. This was premature and I apologize to everyone affected.

I have now been made aware that ert-runner loads that file if it exists, so the file should not be renamed to <package>-test-helper.el as I suggested in those issues. However I still think these elisp files should not provide the feature test-helper because they are not actual libraries that are loaded using require but elisp files that contain testing helpers.

ert-runner does not use require to load the test-helper.el of the package being tested. It uses ert-runner--load, which is a wrapper around load, which loads a file using the path to it, unlike require which locates the file matching a feature on the load-path. See ert-runner.el#L198-L201 and ert-runner.el#L165-L168.

@rejeep, as the author of ert-runner, do you agree that the test-helper.el files should not provide a feature?

others, until we have come to a conclusion here, you should not rename test-helper.el and also delay dropping the (provide 'test-helper).

[DarwinAwardWinner]

Is (require 'test-helper) ever actually run? If not, does it actually cause problems when multiple test-helper.el files call (provide 'test-helper)? Also, is each package's test-helper.el file being installed and added to the load-path when that package in installed via e.g. MELPA? As far as I can tell, it's not. So unless I'm missing something, it seems that although it's most likely wrong to do `(provide 'test-helper), the practical impact should be nil.

[bmag]

Just reporting that (as expected) commenting-out provide doesn't break my tests

[tarsius]

Is (require 'test-helper) ever actually run?

It is not, so yes, (provide 'test-helper) should in most cases cause no harm. However the sole purpose of (provide FEATURE) is to allow (require FEATURE).

The existence of (provide 'test-helper) implies to the reader of test-helper.el that (require 'test-helper) is something that makes sense. I think the cost of (provide 'test-helper) is "non-nil", because it strongly implies that test-helper is a library intended to be loaded with require.

That cost may be somewhat hypothetical - just because it is wrongly implied that (require 'test-helper) is a reasonable thing to do, that doesn't necessarily mean that someone will do it. Still I think that this means that the cost is not non-existent, while I think that there is no benefit.

[rejeep]

do you agree that the test-helper.el files should not provide a feature?

Yes. Same goes for all test files.

As you say, this is only an issue if require is used. So I guess the solution is to not use require, but removing provide is preferable to avoid confusion.

[tarsius]

@rejeep Thanks for confirming this. I have waited a little for others to comment and am now going to update the above issues to suggest the removal of the provide form.

Same goes for all test files.

I think it is okay to provide a feature, iff the name of the test file as well as all the symbols it defines are properly prefixed. Doing that is a good idea regardless of whether a feature is provided or not, many users run tests in their interactive emacs session. And regardless of whether that in turn is a good idea, avoiding name conflicts is good practice.

(One reason why I think it sometimes does make sense to run tests in an interactive session, is that the tests may succeed in batch mode, but the package still raises an error in use. The reason might be that another package patches the package being tested, but that isn't loaded in batch mode. If that error can be reproduced when running the tests interactively, then that might make it easier to debug, then solely based on a backtrace from regular use.

I am sure different opinions on this matter exist, and it's not really the issue at hand. Just though I should mention this.)

[xendk]

It seems to me that the issue more with Emacs autoinsert.el.

I can't quite remember what I did, but checking the ert-runner code, it must have created the file, and when I opened it, autoinsert asked if I wanted to autoinsert. Can't speak for all the other packages, but I guess at least some use it like me to all the standard stuff. And it adds the provide.

So i would suggest, to avoid this issue for cropping up again, that ert-runner provide a default template when creating the file.

[tarsius]

So i would suggest, to avoid this issue for cropping up again, that ert-runner provide a default template when creating the file.

@rejeep I would like to second that suggestion.

[rejeep]

Yes, sounds like a good idea! 👍

[tarsius]

@rejeep Could you please assign this issue to me, so that it appears in my version of https://github.com/issues/assigned. I cannot do it right now, but unless you or someone else beats me to it, I'll eventually implement the template.

[tarsius]

Well, turns out I was able to do it now. Please see #40.

[tarsius]

Unfortunately flycheck tells users to add the provide form. On https://github.com/etu/webpaste.el/pull/6 @etu said:

Ok, I just added it because it's an elisp file and flycheck told me to. But it seems to still run the tests without that line so I merged it.

We should probably add a special case to flycheck to prevent that.

[tarsius]

Or add to the template here: ;; Despite what flycheck claims, you should not provide a feature here.

[xendk]

Not sure what triggers flycheck. I'm sure I've seen that error, but right now I have a test-helper file opened, without a provides, and flychecks seems quite happy about it.

[etu]

Tested in my test-helper.el in webpaste.

If I have the comment (but not the provide):

;;; test-helper.el ends here

at the end, flycheck doesn't complain about it.

But if I remove that comment, I get the warning:

The footer should be: (provide 'test-helper)\n;;; test-helper.el ends here

And this was probably the reason for me adding it to my test-helper. And probably the reason for many others as well.

[tarsius]

@etu Thanks for the information. My change in #40 takes care of this then - it adds the "ends here" line.

[tarsius]

@rejeep Users keep adding the provide form. Please have a look at #40 as soon as you can.

[accidentalrebel]

On my end, I did not add the provide line manually. It was automatically done for me while setting up the cask-package-toolset by @AdrieanKhisbe. The package has a template with the provide line at the end found here.

I'm guessing this can be brought up with the package maintainer.

Also, thanks for the good work @tarsius .

[tarsius]

@accidentalrebel Thanks for the information, I have opened a pr to fix that.