Michael Albinus <michael.albinus@gmx.de> writes:

> Morgan Smith <Morgan.J.Smith@outlook.com> writes:
>
>> Hello!
>
> Hi Morgan,
>
Hi Michael,

Apologies for the delay.  I started a new job the day after I sent this
email!

>> Just a little bit of code cleanup.  I hope this is helpful.
>>
>> You'll notice I removed a bit in `debbugs-newest-bugs' that was supposed to
>> temporarily generate a null value.  I'm not entirely certain but I don't
>> believe that actually does anything right?  Immediately after that we make a
>> synchronous soap call anyways and then return that value.
>
> I don't know. See the comment you've removed as well:
>
> --8<---------------cut here---------------start------------->8---
>> -	  ;; Due to `debbugs-gnu-completion-table', this function
>> -	  ;; could be called in rapid sequence.  We cache temporarily
>> -	  ;; the value nil, therefore.
> --8<---------------cut here---------------end--------------->8---
>
> Have you checked that this doesn't happen without the temporary
> setting?

I'm honestly still not sure.  Maybe it's a simple test but I'm still
scared of completion tables from a bad experience a few years back.
I've just made some modifications so the same logic is in place.

>
> Some few comments:
>
>> index ce6489ca39..d0aa9e87b4 100644
>> --- a/test/debbugs-tests.el
>> +++ b/test/debbugs-tests.el
>> @@ -28,6 +28,10 @@
>>
>>  (require 'debbugs)
>>
>> +;; TODO: This shouldn't be necessary but I get the error
>> +;; "(void-variable debbugs-gnu-use-threads)" without this
>> +(require 'debbugs-gnu)
>
> Oops. This is an existing bug. I will try to fix it later.
>

Thanks for fixing that!

>> +(add-function
>> + :around (symbol-function #'debbugs-get-cache)
>> + #'debbugs-test--override-float-time)
>> +
>> +(add-function
>> + :around (symbol-function #'debbugs-put-cache)
>> + #'debbugs-test--override-float-time)
>
> This is wrong as-it-is. The extended definitions of debbugs-*-cache will
> persist after running the tests. A strict rule is, that all changes must
> be reverted after running the tests.

This is being violated in the current tests by the stubbing out of
`soap-invoke-internal'.  Please see the first attached patch where I fix
this.

> I don't believe you need this. Keep the (cl-letf (((symbol-function
> #'float-time) ...))) clause in debbugs-test-get-status, and do the same
> in debbugs-test-newest-bug-cached.

I wanted a clean wrapper around the cache functions to make it easy for
future testing.  Specifically, I want to add a feature so the cache
survives an emacs restart.

> Otherwise, it looks good to me. Please push to the debbugs repo; I'll
> play with it for a while before making a new release.

I don't believe I have push access to the repository.

> Thanks, and best regards, Michael.

Thanks for reviewing my patches!

Morgan