Fix failing automap test

[jzohrab]

This commit builds on PR #139, and so may need to be rebased and re-issued if those commits are squashed.

The only commit that matters in this branch is the last one, 3bc5824. In summary, this commit lets us refer to test results by human-readable string, rather than a hash, and we can use that human-readable string as the lookup key when running the tests.

I'm not a Lisp programmer, so if there's a better way to write this commit, let me know. Cheers! z

[alphapapa]

Thanks for looking into this. I'd rather not add an override argument like that. I'd rather try to understand what changed about hashing lambdas and fix that if possible. It might even need to be reported upstream.

[jzohrab]

Right, understood. Here are two reasons why you may wish to reconsider this proposed change:

• The change makes explicit the correspondence between each test and its expected result. As a newcomer I had no idea which results.el entry corresponded to the failing test. If the overridehashkey variable name was changed to something like test-result-name, it would make the code clearer and self-documenting.
• If the lambda hashing changed, it may change again in the future. Coupling your code to upstream conventions may cause trouble later. Using self-defined tokens will ensure it doesn't bust.
• (bonus third reason) It works, and gets the tests passing again, with minimal fuss. :-P

It's up to you though! If you want to keep the current method, there may be a far better way to deal with that constraint ... I'm not a real Lisper and so can't say.

[alphapapa]

Right, understood. Here are two reasons why you may wish to reconsider this proposed change:

• The change makes explicit the correspondence between each test and its expected result. As a newcomer I had no idea which results.el entry corresponded to the failing test. If the overridehashkey variable name was changed to something like test-result-name, it would make the code clearer and self-documenting.

A hash is used because the grouping forms are too large to use as human-readable keys anyway. For example, consider this test:

(ert-deftest org-super-agenda-test--forward-looking ()
  (should (org-super-agenda-test--run
           :groups '((:name "Schedule"
                            :time-grid t)
                     (:name "Today"
                            :scheduled today)
                     (:name "Habits"
                            :habit t)
                     (:name "Due today"
                            :deadline today)
                     (:name "Overdue"
                            :deadline past)
                     (:name "Due soon"
                            :deadline future)
                     (:name "Unimportant"
                            :todo ("SOMEDAY" "MAYBE" "CHECK" "TO-READ" "TO-WATCH")
                            :order 100)
                     (:name "Waiting..."
                            :todo "WAITING"
                            :order 98)
                     (:name "Scheduled earlier"
                            :scheduled past)))))

Now imagine trying to find the test with that particular form in the results file. And it won't be formatted that way, it will be on one long line, and visual-line-mode won't make it easier.

Adding names to the tests is an idea. Or, rather than adding names, maybe the test definition names could be used. However, each ERT test definition may contain multiple tests, and each one must have its result individually keyed. So simply using the ERT test name wouldn't work directly. And manually defining names for each test form would be another thing for the programmer to have to keep in sync with the test, ensure there are no duplicates or conflicts, etc.

Hashing solves all of that. It's only now with this change in Emacs 27 that changed how lambdas are hashed that has caused a minor problem. We should try to understand how to fix that properly rather than making potentially unnecessary changes to the test harness.

As far as looking up results in the test file, the programmer isn't intended to do that, anyway. There are commands for show-this-result, save-this-result, and update-all. Those can be run interactively in Emacs while developing, and they can be called from batch mode in scripts and the Makefile. Then the diff for results.el can be double-checked in Magit before committing.

• If the lambda hashing changed, it may change again in the future. Coupling your code to upstream conventions may cause trouble later. Using self-defined tokens will ensure it doesn't bust.

This is Emacs and Org. Everything is coupled to upstream conventions. That's the nature of the beast. Dealing with churn comes with the territory. Fortunately, when it comes to Emacs itself, and especially deep code like sxhash, changes like this are rare.

• (bonus third reason) It works, and gets the tests passing again, with minimal fuss. :-P

I'm not so concerned about one failing test on one version of Emacs when I understand the reason for it and know that it's a false negative. It's not worth making changes to the test harness just for that, at least not until we know that there's not a cleaner way to handle the change. I wrote the test suite and the CI script, and the output is easy to read, and consulting it is not something I do often anyway, because the package is fairly mature now and doesn't change that often.

It's up to you though! If you want to keep the current method, there may be a far better way to deal with that constraint ... I'm not a real Lisper and so can't say.

I appreciate your interest in contributing improvements. This particular problem is not much of a problem, and changes have a cost as well, so I'd prefer to fix it "properly," as best as "properly" can be determined.

[jzohrab]

Hi @alphapapa, thanks for your time with this response. While I've done a lot of TDD in the past, I've not done it with Lisp or ERT, and so don't know the ins and outs. I actually wasn't able to get tests running locally, or even linting (working on a Mac), hence my reliance on CI.

For the human-readable test result name, I've run into the situation you've described in the past. Managing test data can be hard. The solution we used was just adding short tokens to the test data added to the generated test name, e.g. "org-super-agenda-test--forward-looking-a", "org-super-agenda-test--forward-looking-b" etc.

Thanks again. Maybe keep this in your back pocket. I don't have sufficient expertise with this language to get in deep, and if it's not a big deal then this can wait. Cheers! z

[alphapapa]

Thanks.