Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.addError() not called on SkipTest, etc. #45

Open
jpellerin opened this issue Dec 14, 2011 · 18 comments
Open

.addError() not called on SkipTest, etc. #45

jpellerin opened this issue Dec 14, 2011 · 18 comments
Assignees
Milestone

Comments

@jpellerin
Copy link
Member

What steps will reproduce the problem?

  1. Write a plugin, P, that uses .addError
  2. Raise SkipTest in a test

What is the expected output? What do you see instead?

Expected: .addError() is called on P
Got: .addError() is not called on P (assuming
nose.plugins.skip.Skip.addError is called before P.addError by the plugin
manager)

See attached patch for a proper test.

Please use labels and text to provide additional information.

As well as having .addError called, we also still need an interface for
plugins to find out whether an error is ignored. For example, plugin debug
needs this, so that it doesn't try to drop into the debugger on errors that
won't cause an unsuccessful test run, like skips. ATM, this is done by
returning False, which is what stops other plugins' .addError() methods
from getting called.

Discussion:
http://groups.google.com/group/nose-dev/browse_thread/thread/b1d726998dc79bd9

Google Code Info:
Issue #: 169
Author: j...@pobox.com
Created On: 2008-03-10T23:47:10.000Z
Closed On:

@jpellerin
Copy link
Member Author

Google Code Info:
Author: j...@pobox.com
Created On: 2008-03-10T23:47:47.000Z

@ghost ghost assigned jpellerin Dec 14, 2011
@jpellerin
Copy link
Member Author

I also get the same issue: I am using nosetests v0.10.4

Did anyone understand what is going on?

Thanks,
Vincent

Google Code Info:
Author: vincent....@gmail.com
Created On: 2009-08-13T14:13:07.000Z

@jpellerin
Copy link
Member Author

Still doesn't work. Using nose 0.11.3 and Python 2.7 on Fedora 14.

Below is a patch that calls addSkip() in the plugin. I realize the manual says it's deprecated, but it works for me.

--- result.py.orig 2010-12-20 11:54:06.904684002 +0100
+++ result.py 2010-12-20 11:33:30.596684002 +0100
@@ -50,6 +50,7 @@
storage, label, isfail = self.errorClasses[SkipTest]
storage.append((test, reason))
self.printLabel(label, (SkipTest, reason, None))

  •    self.config.plugins.addSkip(test)
    

    def addError(self, test, err):
    """Overrides normal addError to add support for

Google Code Info:
Author: geertj@gmail.com
Created On: 2010-12-20T10:55:33.000Z

@jpellerin
Copy link
Member Author

I still have this problem on nose 1.1.2. Neither addSkip nor addError is called to handle a Skip. Same goes for DeprecatedTest.

Google Code Info:
Author: pettazz
Created On: 2011-11-23T00:43:26.000Z

Bahus added a commit to Bahus/nose that referenced this issue Jun 5, 2012
@Bentis
Copy link

Bentis commented Sep 18, 2012

Still a problem in nose 1.2.0, however I quickfixed this by giving my plugin a score above that of the Skip/Deprecated/Custom error class plugins (default score of 1000). That way my plugin gets the addError call before the error class which suppresses it.

wiggin15 added a commit to Infinidat/nose that referenced this issue Apr 8, 2013
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
@wiggin15
Copy link
Contributor

wiggin15 commented Apr 8, 2013

this was broken since 773dc1c - looks like it was intentional to prevent --pdb from triggering on Skip errors. The problematic code is now in errorclass.py - since its score is high and it returns True in addError it prevents any other plugins from getting addError in case of Skip\Deprecated.
It shouldn't do that, the fix would be to remove the addError handler in errorclass.py and prevent --pdb from triggering by checking for Skip\Deprecated in the pdb plugin itself. It will be a bit ugly to check for the specific exceptions in Pdb.addError, but unfortunately that's what all plugins that are sensitive to Skip have to do, now that addSkip and addDeprecated are deprecated. That's what the plugin documentation suggests (IMHO addSkip shouldn't be deprecated, and SkipTest shouldn't cause an addError call, but looks like that decision has been made).

@SebaM
Copy link

SebaM commented Feb 24, 2014

Why this patch wasn't marged w/ master? About one year later it could be still problem. Eg. in my case I need upload test data to db so I'm using nosedbreport plugin. The problem I'm encountered was not working plugin in case of skip test. After applying this patch works much better.

wiggin15 added a commit to Infinidat/nose that referenced this issue Mar 16, 2014
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
@davydany
Copy link

davydany commented Apr 9, 2014

I'm with @SebaM, can this be merged with master?

@CleanCut
Copy link

@jpellerin I just hit this issue in my plugin as well. I agree with @SebaM and @davydany -- can we please merge the patch from @wiggin15 or some other relevant fix? (Pretty please!?) My plugin cannot process skips in it's current version.

If there's a reason you won't merge the patch, please let us know. I will use the workaround @Bentis mentioned if this is not going to get fixed any time soon.

I personally think it is more consistent to either have [addSkip, addFailure, addError] OR have addError called for skips, failures, and errors. Whatever the case, at a minimum it should work as the documentation currently asserts that it works.

My plugin is called green. Anyone interested is welcome to take a look at it

@jszakmeister
Copy link
Contributor

@CleanCut, @SebaM, and @davydany: FWIW, @jpellerin turned over maintainership to me--he's simple too busy. Can we talk about the opposite problem: how many plugins does this break? How many will see errors where they were seeing none before? Do we know what form do those errors take?

And let's talk about the patch itself: why is addError() being removed from the base class for error-related plugins (ErrorClassPlugin)? Why is a test called TestErrorClassWithStringException being removed? Why is the filterError() call in the path for both addFailure() and addError() instead of just addError()? How about a test to make sure this new behavior sticks?

While nose has a sizable test suite, it doesn't fully cover all edge cases--so it's not enough to simply pass the tests. For example, I believe the removal of addError() breaks the ability to disable the skip and deprecated plugins--a possible regression. Python 2.4 and 2.5 supports raising strings as exceptions, something that 1.3.x still supports. So if merging means regressing, then I can't do it yet--which is why I haven't looked at this more.

There's a sizable plugin ecosystem that I'm worried about breaking since this changes the semantics of the plugin API--fixing an inconsistency only to break most of the plugin ecosystem is not a fix to me. I'm unwilling to merge is for a patch release since it changes plugin behavior--at least not without good arguments showing minimal breakage. So I've been saving it for 1.4.0 release, but I'm not sure when that will happen as I'm currently focused on trying to get a 1.3.2 release out the door. With 1.4.0, I hope we can drop support for 2.5 and below, and possibly 2.6 and below. But we should all keep in mind that Nose is in maintenance mode too. Anyone building new plugins should really be looking at nose2.

I hope that helps, and I hope that all of you can shed some light on the expected fallout and help provide a sound review. Like many in the open source world, I have a day job and a family, so I only get a limited amount of time to spend looking at issues in-depth. Any help you can provide here is useful.

@CleanCut
Copy link

@jszakmeister: Aaarrrg. I was unaware of the existence of nose2. I will pivot and rebase my plugin off of nose2. I'm not concerned with python older than 2.7 anyway. I presume nose2 will have less issues like this.

I highly advise adding a big advisory on the nose home page pointing people to nose2. I've only been using/developing for nose for two weeks, but in my extensive reading and googling I never once came across anything mentioning the existence of a nose2 project. I would have simply started there if I had been aware.

@wiggin15
Copy link
Contributor

Just to defend the patch, in answer to your questions: ErrorClassPlugin.addError was removed because this IS this fix! ErrorClassPlugin is a plugin with a high score with the sole purpose of filtering out "Deprecated" and "Skip" exceptions so that they won't get to the plugins - it was an ugly hack. TestErrorClassWithStringException was removed because it tests something inside ErrorClassPlugin.addError which was now removed -- this is documented in the commit itself, even with a reference to the issue where this test was added... I wish you had reviewed the commit properly before dissing it so impolitely...
As for the filter in both addError and addFailure, I'm not sure about this one, I think there was a reason but maybe it can be changed. The question is if any error classes other than AssertionError can get to addFailure...

I agree that there is an issue with existing plugins. In my opinion the real fix will be to bring back addSkip and addDeprecated - this will not break compatibility and be much easier for plugins to handle...
Even if you will not accept this patch, this is still a problem, as the documentation specifically state that the exceptions should get to addError (http://nose.readthedocs.org/en/latest/plugins/interface.html), and in addition, currently plugins have no way of being notified of these events...

As for nose2: I don't see this project as a reason to drop maintanence of nose, with so many users still using nose and its plugins and so few users and plugins switching to nose2 - I just don't think it ever took off (nose is ranked 13th in pypi, nose2 is ranked 3735th). I don't think we're going to agree here, though.

@jszakmeister
Copy link
Contributor

Just to defend the patch, in answer to your questions: ErrorClassPlugin.addError was removed because this IS this fix! ErrorClassPlugin is a plugin with a high score with the sole purpose of filtering out "Deprecated" and "Skip" exceptions so that they won't get to the plugins - it was an ugly hack.

It appears to me that any plugin implemented on that base is relying on the filtering that was removed to support the --no- variants of the plugin. So, it appears that this patch breaks that functionality.

TestErrorClassWithStringException was removed because it tests something inside ErrorClassPlugin.addError which was now removed -- this is documented in the commit itself, even with a reference to the issue where this test was added... I wish you had reviewed the commit properly before dissing it so impolitely...

I'm sorry you saw it as being impolite--that was not my intention. I did read the commit message and I understood that it was supposed to be part of the fix, but the commit message did not say anything to address the other concerns, and unfortunately your response hasn't either. :-( To put it plainly: does --no-skip and --no-deprecated work correctly after this patch?

I agree that there is an issue with existing plugins. In my opinion the real fix will be to bring back addSkip and addDeprecated - this will not break compatibility and be much easier for plugins to handle...

That depends on your view of compatibility. It's certainly a change in behavior, and different than what is happening currently (and necessarily so), so it could be seen as a break in compatibility which is why I believe it needs to be handled delicately.

Even if you will not accept this patch, this is still a problem, as the documentation specifically state that the exceptions should get to addError (http://nose.readthedocs.org/en/latest/plugins/interface.html), and in addition, currently plugins have no way of being notified of these events...

I agree it's a problem and that it should be fixed, but I need to understand the implications. And yes, the documentation should get updated if we're going to keep it in the current form.

As for nose2: I don't see this project as a reason to drop maintanence of nose, with so many users still using nose and its plugins and so few users and plugins switching to nose2 - I just don't think it ever took off (nose is ranked 13th in pypi, nose2 is ranked 3735th). I don't think we're going to agree here, though.

Nose has some serious issues that cannot be fixed without serious effort (like plugins interacting with multiprocessing). Nose2 is built on a much better core, and doesn't have all the compatibility concerns. I'd love to say that Nose will get there, but it's hard to see the value in it since unittest has gotten so much better and Nose2 addresses the remaining issues.

@jszakmeister jszakmeister added this to the 1.4.0 milestone Apr 18, 2014
@jszakmeister
Copy link
Contributor

@wiggin15 So I spent some time with this on the plane. The good news is that your patch doesn't break --no-skip and --no-deprecate, nor does it break using string exceptions (though we've had an issue there on master for a while--since 2009?!--but it's now fixed).

However, there are some other considerations. We talk about the ErrorClassPlugin in the documentation (basically offering it up for others to build on). I think it might be better to just add an empty addError to the Skip and Deprecate plugins to avoid breaking anyone who might be depending ErrorClassPlugin--though it probably means that nothing in Nose is really using that functionality anymore.

It also turns out that we have some interesting some interesting behavior, at least for the Skip plugin when using --no-skip. On Python 2.7 or better, with the skip plugin disabled, instead of getting an error, we get no explanation about the test (the line is left blank after the ...). I imagine this is because of unittest's native ability to skip tests in 2.7. At any rate, the behavior was unchanged by your patch.

@ghost
Copy link

ghost commented Apr 24, 2014

Just read this entire conversation as I'm running into this issue as well.

So, what is a plugin developer who wants to handle skipped test to do, if they want to distribute their plugin and not ask users to merge the Nose patch manually?

I'm currently writing a plugin and need to handle skipped tests. For now, I just set the score of my Plugin to a value >1000. Is this the accepted practice?

@CleanCut
Copy link

@gardaud From my perspective, setting score > 1000 is the only option at the moment. At least if you're sticking with nose.

Since my project is mostly to scratch my own itch, I'm just moving to nose2. I'm liking nose2 so far -- plugin development is far cleaner. I almost have green refactored to work with it. (I haven't pushed up my refactoring work yet, though. It will be version 0.9.0)

@CleanCut
Copy link

CleanCut commented May 2, 2014

I discovered that it was actually much more straightforward (and less buggy -- no issues like this one to deal with) to just take the example TextTestRunner and TextTestResult from unittest and write new versions than to try to adapt to nose's or nose2's plugin API and behavior. So Green is now a standalone test runner.

Green is runnable now. It doesn't aspire to have all the features that nose or nose2 have, but it's working pretty well for me. If anyone wants to check it out I would welcome feedback.

@jcelliott
Copy link

I'm running into this issue as well. My plugin uses custom TestRunner and TestResult classes, so setting the plugin score >1000 doesn't do anything. Is there any way to work around this (aside from using nose2) so the test result gets the .addError and .AddSkip calls?

wiggin15 added a commit to Infinidat/nose that referenced this issue Jul 31, 2014
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
wiggin15 added a commit to Infinidat/nose that referenced this issue Oct 6, 2014
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
wiggin15 added a commit to Infinidat/nose that referenced this issue Mar 18, 2015
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
wiggin15 added a commit to Infinidat/nose that referenced this issue Apr 5, 2015
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
wiggin15 added a commit to Infinidat/nose that referenced this issue Aug 4, 2015
ErrorClass returns True on addError to prevent SkipTest and
DeprecatedTest from getting to the pdb plugin. However this prevents
all plugins from getting them, and the documentation specifies that
these exceptions should get to the plugins. The pdb plugin should handle
these exceptions explicitly as the documentation suggests.
This also deprecates Google Code Issue nose-devs#101 so there is nothing to test
there anymore.
This fixes nose-devs#45.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants