Code review comment for lp:~javier.collado/mago/suite_discovery

Revision history for this message
Javier Collado (javier.collado) wrote :

Hello Eitan,

Thanks for your comments and your feedback. I'm afraid that the code is not free from defects so I really appreciate your help.

I've taken a look at that piece of code and the original version still looks fine to me. Let me explain it as clear as possible:
- suite is a SuiteData object that implements the __eq__ method for comparisons
- suite.name is the name of the test suite as written in the xml file
- suite.filename is the name of the xml file that contains the suite description data

My understanding was that in the main branch the suite filtering isn't performed by the suite name, but by the suite filename (with or without extension). Is that correct? In such a case that piece of code seems to be right since it's working that way thanks to the __eq__ method. For example:

(Pydb) suite
<desktoptesting.cmd.discovery.SuiteData instance at 0x8aead4c>
(Pydb) suite.name
'gedit chains'
(Pydb) suite.filename
'gedit_chains.xml'
(Pydb) suite in ['gedit chains']
False
(Pydb) suite in ['gedit_chains.xml']
True
(Pydb) suite in ['gedit_chains']
True

Please let me know if this is the way suite filtering is expected to work?

Anyway, I'll take a look at the safe_run_command output to make sure what happened to the logs.

Best regards,
    Javier

> Wasn't a logger bug, here is the fix:
>
> === modified file 'desktoptesting/cmd/discovery.py'
> --- desktoptesting/cmd/discovery.py 2009-06-09 07:23:10 +0000
> +++ desktoptesting/cmd/discovery.py 2009-06-16 21:21:15 +0000
> @@ -243,7 +243,7 @@
> # command line
> if cls.whitelist:
> suites = (suite for suite in suites
> - if suite in cls.whitelist)
> + if suite.name in cls.whitelist)
>
> return suites

« Back to merge proposal