Possible bug in pluginsController.registerOneHandler

[vitalije]

# from leoPlugins.py file

def registerOneHandler(self, tag, fn):

    """Register one handler"""

    try:

        moduleName = self.loadingModuleNameStack[-1]

    except IndexError:

        moduleName = '<no module>'

    ...

    items = self.handlers.get(tag, [])

    if fn not in items:

       bunch = g.Bunch(fn=fn, moduleName=moduleName, tag='handler')

       items.append(bunch)

    self.handlers[tag] = items

It looks like the above code checks to see if given function has already been registered, (`if fn not in items:`) but then it puts a bunch instance in items not the handler function. Unless I am mistaken, it will never be the case that the above condition is False, because handler function will never be added to items array.
Vitalije

[Edward K. Ream]

​Good catch. The test above looks like mistaken defensive programming. We could eliminate the test entirely, or test each bunch in items:  if bunch.fn == fn:...

Care to change this yourself?

Edward

[vitalije]

On Wednesday, April 5, 2017 at 7:24:15 PM UTC+2, Edward K. Ream wrote:

​Good catch. The test above looks like mistaken defensive programming. We could eliminate the test entirely, or test each bunch in items:  if bunch.fn == fn:...

Care to change this yourself?

Edward

I'll try ASAP (in a day or two).
Vitalije

[Edward K. Ream]

​Thanks.

Edward

[vitalije]

I tried to `git push` my corrections but was refused with status 403.

So I forked leo-editor and pushed changes to my fork of leo-editor. I have added two new @test nodes in leo/test/unitTest.leo that test expected behavior of registerOneHandler and registerOneExclusiveHandler. After that I have made some corrections in leo/core/leoPlugins.py to make new tests pass.

However, when I tried to run all unit tests to check that I didn't broke something else in Leo codebase, I've got more than 100 failed tests?! Am I missing something? What is the correct way to run all tests before committing changes to main branch?

Besides, method registerOneExclusiveHandler of leoPluginsController was even more broken. It is most unlikely that any plugin used this method ever because it was replacing the handlers map with the array of one bunch containing one exclusive handler.

def registerOneExclusiveHandler(self, tag, fn):

   ....

if tag in self.handlers:

g.es("*** Two exclusive handlers for", "'%s'" % (tag))

else:

bunch = g.Bunch(fn=fn, moduleName=moduleName, tag='handler')

       self.handlers = [bunch]

The last line would broke all other plugins.


Vitalije

[vitalije]

Pull request is here
Vitalije

[Terry Brown]

On Thu, 6 Apr 2017 03:49:25 -0700 (PDT)
vitalije <vita...@gmail.com> wrote:

> However, when I tried to run all unit tests to check that I didn't
> broke something else in Leo codebase, I've got more than 100 failed
> tests?! Am I missing something? What is the correct way to run all
> tests before committing changes to main branch?

When I run unit tests I usually get about 5-15 failures. This is
tricky, because it means you have to run them before and after your
change to make sure nothing new has been broken. In fact it's a major
impediment to using unit tests that probably requires a major project
using clean VM installs to pin down an environment in which all the
tests work. And / or some use of Travis CI or something similar.

Don't know why you'd have seen 100 fails - I run the tests by loading
leo/test/unitTest.leo, selecting the Active Unit Tests node, and pressing
Alt-4.

Cheers -Terry

[Edward K. Ream]

On Thu, Apr 6, 2017 at 8:39 AM, 'Terry Brown' via leo-editor <leo-e...@googlegroups.com> wrote:

On Thu, 6 Apr 2017 03:49:25 -0700 (PDT)
vitalije <vita...@gmail.com> wrote:

​Thanks, Vitalije, for your work.  e2ad1d1 contains it, slightly modified.  All my unit tests pass.

I've marked changes with # Vitalije.

Edward