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

Proposed by Eitan Isaacson on 2009-08-17
Status: Merged
Merged at revision: not available
Proposed branch: lp:~eeejay/mago/blacklist
Merge into: lp:~mago-contributors/mago/mago-1.0
Diff against target: None lines
To merge this branch: bzr merge lp:~eeejay/mago/blacklist
Reviewer Review Type Date Requested Status
Ara Pulido Approve on 2009-09-23
Joker Wild (community) general overview Approve on 2009-08-25
James Tatum Approve on 2009-08-19
Javier Collado (community) Approve on 2009-08-19
Nagappan Alagappan 2009-08-17 Approve on 2009-08-17
Review via email: mp+10271@code.launchpad.net
To post a comment you must log in.
Eitan Isaacson (eeejay) wrote :

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 updated on 2009-08-17
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) wrote :

Good to go

review: Approve
Javier Collado (javier.collado) wrote :

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) wrote :

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) 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.

Eitan Isaacson (eeejay) wrote :

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) wrote :

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

lp:~eeejay/mago/blacklist updated on 2009-08-18
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) wrote :

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

review: Approve
James Tatum (jtatum) wrote :

Nice!

review: Approve

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

review: Approve (general overview)
Ara Pulido (ara) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'mago/cmd/discovery.py'
2--- mago/cmd/discovery.py 2009-08-04 10:27:46 +0000
3+++ mago/cmd/discovery.py 2009-08-17 19:56:55 +0000
4@@ -236,6 +236,20 @@
5 return (self.tree
6 and self.tree.getroot().tag == 'suite')
7
8+ @property
9+ def blacklisted(self):
10+ """
11+ Return True if this case should not be run.
12+ """
13+ whitelisted = bool(self.name_whitelist and \
14+ self.name in self.name_whitelist)
15+
16+ whitelisted |= bool(self.filename_whitelist and \
17+ self in self.filename_whitelist)
18+
19+ blacklisted = self.root.find('blacklisted') is not None
20+
21+ return blacklisted and not whitelisted
22
23 @classmethod
24 def discover(cls, app):
25@@ -283,6 +297,17 @@
26 return self.root.find('method').text
27
28
29+ @property
30+ def blacklisted(self):
31+ """
32+ Return True if this case should not be run.
33+ """
34+ whitelisted = bool(self.whitelist and self.name in self.whitelist)
35+
36+ blacklisted = self.root.find('blacklisted') is not None
37+
38+ return blacklisted and not whitelisted
39+
40 @classmethod
41 def discover(cls, suite):
42 """
43
44=== modified file 'mago/cmd/runner.py'
45--- mago/cmd/runner.py 2009-07-21 18:20:22 +0000
46+++ mago/cmd/runner.py 2009-08-17 19:56:55 +0000
47@@ -26,6 +26,11 @@
48 """
49 Run test case and gather results
50 """
51+ if self.case_data.blacklisted:
52+ logging.info("Skipping blacklisted test case '%s'" % \
53+ self.case_data.name)
54+ return
55+
56 logging.info("Running test case '%s' (%s)"
57 % (self.case_data.name,
58 self.case_data.methodname))
59@@ -88,6 +93,11 @@
60 """
61 Run the test suite and return xml report
62 """
63+ if self.suite_data.blacklisted:
64+ logging.info("Skipping blacklisted test suite '%s'" % \
65+ self.suite_data.name)
66+ return etree.tostring(self.suite_data.root, "utf-8")
67+
68 logging.info("Running test suite '%s' (%s)"
69 % (self.suite_data.name,
70 self.suite_data.fullname))

Subscribers

People subscribed via source and target branches

to status/vote changes: