search

emacs-compat / compat

COMPATibility Library for Emacs Lisp
https://elpa.gnu.org/packages/compat.html
GNU General Public License v3.0
69 stars 12 forks source link

Unbreak test package-get-version #15

Closed aagon closed 1 year ago

aagon commented 1 year ago

Hello Daniel,

The test package-get-version can fail if the directory storing the elisp files is not called "compat".

The steps to reproduce should be the following : 

git clone https://github.com/emacs-compat/compat randomdirectoryname
cd randomdirectoryname
make test

Only this : 

git clone https://github.com/emacs-compat/compat
cd compat
make test

works. This is because package-get-version relies as a last resort on the directory name to get the "good" main file of the package.

I propose to set the name of the "main file" from which to read the version number ourselves. It is always the exact source file providing the compat feature required by the tests, so it should work every time.

Then we can get the version number of the package like package-get-version would.

Of course, there are probably multiple other ways to get the version number of the package, like some user-facing function. This is just one way of doing it.

Best,

Aymeric

minad commented 1 year ago

Thanks for the report. Your patch is not the correct way to fix this, since you effectively disabled the test. The whole point is to test package-get-version. You can try to fix this differently, e.g., by disabling the test depending on external flags. But generally I won't accept any patches which weaken the test suite. The whole robustness of the package depends on it.

aagon commented 1 year ago

The whole point is to test package-get-version.

I did not think of that, I thought the point was to test the version itself (which I admit does not make a lot of sense).

You can try to fix this differently, e.g., by disabling the test depending on external flags.

I conditioned the test to the containing directory being named "compat" or "compat-\<version>" in my latest commit.

This allows the test to cover the second and the third path of the conditional inside package-get-version.

It is the best I could think of at the moment.

Let me know what you think, and feel free to pull from it.

Best,

Aymeric

minad commented 1 year ago

Okay, see https://github.com/emacs-compat/compat/commit/108e6d3f6207632d040ea740a4b51b2a14ed38e0.

aagon commented 1 year ago

The change you made reintroduces a path of spurious test failure, for instance if the containing directory is compat-el (full disclosure : this is exactly how we renamed compat in debian, to avoid conflicts with other packages).

I had chosen the regexp so as to match compat or compat-<version>, with version being digits, periods, and potentially "pre" or "alpha", etc... because that is how package archives like elpa or melpa, and also because this is exactly what package-get-version considers to be a "version".

minad commented 1 year ago

Okay, I suggest you maintain a patch as part of the Debian packaging. Compat is still in flux and I don't actually want to maintain any special test skipping criteria. I really care mostly about the CI working correctly. Compat is a problematic package, since it is not bound to a namespace. It must be rock solid and I am in the process of making it such (full test coverage since 29.1.2.0, etc.).

aagon commented 1 year ago

Okay, I suggest you maintain a patch as part of the Debian packaging.

Oh, this is already what we are doing, but we also try to forward our patches upstream, and since we have quite uncommon build environments, we tend to uncover some unlikely corner cases that upstream might want to hear about, that is all.

I don't actually want to maintain any special test skipping criteria. I really care mostly about the CI working correctly.

This is perfectly understandable. Since you control the test environment totally, the skipping logic is of little interest to you. But since it was actually quite easy for your users to fail that test by simply deciding not to go with the default name of the repo, I felt compelled to mention it. I should have been clearer from the start.

Compat is a problematic package, since it is not bound to a namespace. It must be rock solid and I am in the process of making it such (full test coverage since 29.1.2.0, etc.).

Agreed. For the time being, only one of our debian packages (one of yours actually, consult) depends on compat, but we will have many more in the future (magit, for instance). It being rock solid is also of primary importance to us.

Thank you very much for your time and your work !

Aymeric

minad commented 1 year ago

Yes, I appreciate the work you are doing. It is just not the right time for such details which are not of primary importance for my current goals. I expect things to cool down in a month or two and then we can address all weird corner cases. I will be happy to hear about them.

Compat will probably also get adopted by Org and maybe by other core packages for their ELPA versions. ERC already uses Compat, but afaik not yet in an official ELPA release.