Merge lp:~eeejay/mago/blacklist into lp:~mago-contributors/mago/mago-1.0

[Eitan Isaacson]

[Eitan Isaacson (eeejay)]

This branch answers the need for blacklisting specific cases and suites. This might be useful for disabling tests that are known to be broken and run amok. It is also useful for "special" tests like the ubiquity one that is in the pipe.

[lp:~eeejay/mago/blacklist]

104. By Eitan Isaacson on 2009-08-17

don't load a module if it is blacklisted

105. By Eitan Isaacson on 2009-08-17

merge from trunk

106. By Eitan Isaacson on 2009-08-17

merge from trunk

[Nagappan Alagappan (nagappan)]

Good to go

review: Approve

[Javier Collado (javier.collado)]

Hello Eitan,

It seems to me that the changes aren't consistent with the previous design. Given that the whitelisting is performed in the discover method, why not doing the same thing for the blacklist?

The idea is that the discovery does all the filtering and the runner doesn't have to worry about those details. Do you agree on this?

Best regards,
    Javier

review: Needs Information

[Eitan Isaacson (eeejay)]

I agree. The problem is that '-i' will permanently hide tests, and make it
hard to discover them for explicit running.

There is a need for a non-command line blacklisting, because the list of
unrunnable tests is extensive, and does not change very often. I think
defining it in the XML is a good place for it since we are storing all the
test's data there.

Maybe the term 'blacklist' is wrong here. Maybe we should use the term
'skip'? Also, when running '-i' is should show '(skip)' as a suffix to the
case/suite.

What do you think?

On Tue, Aug 18, 2009 at 5:08 AM, Javier Collado <
<email address hidden>> wrote:

> Review: Needs Information
> Hello Eitan,
>
> It seems to me that the changes aren't consistent with the previous design.
> Given that the whitelisting is performed in the discover method, why not
> doing the same thing for the blacklist?
>
> The idea is that the discovery does all the filtering and the runner
> doesn't have to worry about those details. Do you agree on this?
>
> Best regards,
> Javier
> --
> https://code.launchpad.net/~eeejay/mago/blacklist/+merge/10271<https://code.launchpad.net/%7Eeeejay/mago/blacklist/+merge/10271>
> You are the owner of lp:~eeejay/mago/blacklist.
>

[Javier Collado (javier.collado)]

Uhm, now I see what you wanted to do. Maybe changing the name from blacklist to skip is a good idea
because I saw the change as implementing the complementary functionality to the already implemented
whitelist. I understand that it might be needed to add something in the xml to skip/ignore/... a
test case.

[Eitan Isaacson (eeejay)]

Yeah, I think we agree. I will make those changes soon, and comment on this
request when done.

On Tue, Aug 18, 2009 at 9:32 AM, Javier Collado <
<email address hidden>> wrote:

> Uhm, now I see what you wanted to do. Maybe changing the name from
> blacklist to skip is a good idea
> because I saw the change as implementing the complementary functionality to
> the already implemented
> whitelist. I understand that it might be needed to add something in the xml
> to skip/ignore/... a
> test case.
> --
> https://code.launchpad.net/~eeejay/mago/blacklist/+merge/10271<https://code.launchpad.net/%7Eeeejay/mago/blacklist/+merge/10271>
> You are the owner of lp:~eeejay/mago/blacklist.
>

[Eitan Isaacson (eeejay)]

Ok, I did the proposed changes, from 'blacklisted' to 'skipped', also '-i' now shows which tests will be skipped.

[lp:~eeejay/mago/blacklist]

107. By Eitan Isaacson on 2009-08-18

change 'blacklisted' to 'skip'. Suffix 'SKIP' to tests that will be skipped in -i

[Javier Collado (javier.collado)]

Now it looks fine to me. Thanks for your perseverance.

review: Approve

[James Tatum (jtatum)]

Nice!

review: Approve

[Joker Wild (lajjr-deactivatedaccount)]

eeejay, Yes Nice to blacklist tests that don't work well.
Regards,
Leo Jackson
lajjr

review: Approve (general overview)

[Ara Pulido (ara)]

Eitan, a couple of things:

I don't understand why you use the discovery whitelist to ask whether a test case should be run or not.

I understand whitelist in discovery, as the list of cases that match the condition to be run (application, suite name...) and not as the opposite of the thing you want to do.

So, i.e. I was skipping gedit_chains by putting <skip>Yeah</skip> in the suite level.

If I do:

PYTHONPATH=. ./bin/mago -a gedit, the suite is skipped
but if I do
PYTHONPATH=. ./bin/mago -f gedit_chains, the suite is run

I would have expected both to be skipped. The skip value, as it is in the XML should not be related to the command line parameters. It should be something else that ignores the command line and just skip the test case (or suite)

review: Needs Fixing

[Javier Collado (javier.collado)]

We all may have different expectations regarding what should be the effect of a tag in an xml file. I'd say that what Eitan wanted to implement was a way to skip a few test suites by default unless explicitly whitelisted and that's, I believe, what the code does.

If the test suite was to be skipped under all circumstances, then it wouldn't be possible to run it without editing the xml file and I guess that's another thing that Eitan was trying to avoid.

Maybe the 'skip' name is misleading and a different one should be implemented such as 'skip_by_default'.

Anyway, my interpretation of the change might not be correct.

[Eitan Isaacson (eeejay)]

Yes, Javier's interpretation is mine too. 'skip' means skip by default, if the case or suite is explicitly called, it should be run. This allows you to work on a test and not remove the 'skip' until it is ready for inclusion. Or in Ubiquity's case, to run the test when explicitly called on not in a general Mago run.

[Ara Pulido (ara)]

OK, I misunderstood the change.
I will merge now and will try to document the change properly, to avoid more misunderstandings :-)

Thanks Eitan!

review: Approve

=== modified file 'mago/cmd/discovery.py'
--- mago/cmd/discovery.py	2009-08-04 10:27:46 +0000
+++ mago/cmd/discovery.py	2009-08-17 19:56:55 +0000
@@ -236,6 +236,20 @@
         return (self.tree
                 and self.tree.getroot().tag == 'suite')
 
+    @property
+    def blacklisted(self):
+        """
+        Return True if this case should not be run.
+        """
+        whitelisted = bool(self.name_whitelist and \
+                               self.name in self.name_whitelist)
+
+        whitelisted |= bool(self.filename_whitelist and \
+                                self in self.filename_whitelist)
+
+        blacklisted = self.root.find('blacklisted') is not None
+
+        return blacklisted and not whitelisted
 
     @classmethod
     def discover(cls, app):
@@ -283,6 +297,17 @@
         return self.root.find('method').text
 
 
+    @property
+    def blacklisted(self):
+        """
+        Return True if this case should not be run.
+        """
+        whitelisted = bool(self.whitelist and self.name in self.whitelist)
+
+        blacklisted = self.root.find('blacklisted') is not None
+
+        return blacklisted and not whitelisted
+
     @classmethod
     def discover(cls, suite):
         """

=== modified file 'mago/cmd/runner.py'
--- mago/cmd/runner.py	2009-07-21 18:20:22 +0000
+++ mago/cmd/runner.py	2009-08-17 19:56:55 +0000
@@ -26,6 +26,11 @@
         """
         Run test case and gather results
         """
+        if self.case_data.blacklisted:
+            logging.info("Skipping blacklisted test case '%s'" % \
+                             self.case_data.name)
+            return
+
         logging.info("Running test case '%s' (%s)"
                      % (self.case_data.name,
                         self.case_data.methodname))
@@ -88,6 +93,11 @@
         """
         Run the test suite and return xml report
         """
+        if self.suite_data.blacklisted:
+            logging.info("Skipping blacklisted test suite '%s'" % \
+                             self.suite_data.name)
+            return etree.tostring(self.suite_data.root, "utf-8")
+
         logging.info("Running test suite '%s' (%s)"
                      % (self.suite_data.name,
                         self.suite_data.fullname))