Launchpad itself

Code review comment for lp:~mars/launchpad/disable-windmill-tests

disable-windmill-tests
Merge into devel

Revision history for this message

Gary Poster (gary) wrote on 2010-05-17:

[3:03pm] gary_poster: mars, you added in TESTOPTS for theoretical correctness, yes?
[3:03pm] gary_poster: that is, no one is actually using them, it seems?
[3:03pm] mars: gary_poster, it was already in there, just not used properly.
[3:04pm] mars: they are used by three targets lower down in the file
[3:04pm] gary_poster: ah, gotcha mars
[3:04pm] mars: I was going to rename VERBOSITY to TESTOPTS myself, and was surprised to find TESTOPTS was already in there
[3:05pm] gary_poster: yeah, another approach would be to rip it out, but I suppose this is safer
[3:05pm] mars: using the value of the VERBOSITY variable to pass "-vvv -t --layer" to the test process really rubbed me the wrong way. Refactoring gene I guess
[3:05pm] mars: gary_poster, safer, yes. I don't know who else used VERBOSITY that way.
[3:06pm] gary_poster: mars, have you checked to see what happens to the test runner if you say --layer='!WindmillLayer' --layer='WindmillLayer'?
[3:06pm] mars: Needed a fast fix. This is one step forward.
[3:06pm] mars: My guess is that --layer=!Layer --layer=Layer *will* execute the desired test suite.
[3:06pm] gary_poster: well, for a fast fix, maybe leaving TESTOPTS out of the picture would be wiser. Otherwise I have questions about it
[3:06pm] mars: But no, I have not explicitly tested that
[3:07pm] mars: gary_poster, oh? What questions?
[3:07pm] gary_poster: for instance--if you are right (which would be my guess) then I would suggest that ``bin/test $(VERBOSITY) $(TESTOPTS) --layer=WindmillLayer`` should instead be ``bin/test $(VERBOSITY) --layer=WindmillLayer $(TESTOPTS)`` so that TESTOPTS can actually override things
[3:07pm] mars: gary_poster, I treat TESTOPTS like CFLAGS. VERBOSITY is independent of the two.
[3:08pm] mars: gary_poster, ok, interesting. That woudl have come up regardless if I used VERBOSITY or TESTOPTS.
[3:09pm] gary_poster: yeah, calling it TESTOPTS is more correct, and makes the possible problem clearer.
[3:09pm] gary_poster: mars, I'll approve, simply asking that you consider my question...possibly later.
[3:09pm] mars: +1

[3:03pm] gary_poster: mars, you added in TESTOPTS for theoretical correctness, yes?
[3:03pm] gary_poster: that is, no one is actually using them, it seems?
[3:03pm] mars: gary_poster, it was already in there, just not used properly.
[3:04pm] mars: they are used by three targets lower down in the file
[3:04pm] gary_poster: ah, gotcha mars
[3:04pm] mars: I was going to rename VERBOSITY to TESTOPTS myself, and was surprised to find TESTOPTS was already in there
[3:05pm] gary_poster: yeah, another approach would be to rip it out, but I suppose this is safer
[3:05pm] mars: using the value of the VERBOSITY variable to pass "-vvv -t --layer" to the test process really rubbed me the wrong way.  Refactoring gene I guess 
[3:05pm] mars: gary_poster, safer, yes.  I don't know who else used VERBOSITY that way.
[3:06pm] gary_poster: mars, have you checked to see what happens to the test runner if you say --layer='!WindmillLayer' --layer='WindmillLayer'?
[3:06pm] mars: Needed a fast fix.  This is one step forward.
[3:06pm] mars: My guess is that --layer=!Layer --layer=Layer *will* execute the desired test suite.
[3:06pm] gary_poster: well, for a fast fix, maybe leaving TESTOPTS out of the picture would be wiser.  Otherwise I have questions about it
[3:06pm] mars: But no, I have not explicitly tested that 
[3:07pm] mars: gary_poster, oh?  What questions?
[3:07pm] gary_poster: for instance--if you are right (which would be my guess) then I would suggest that ``bin/test $(VERBOSITY) $(TESTOPTS) --layer=WindmillLayer`` should instead be ``bin/test $(VERBOSITY) --layer=WindmillLayer $(TESTOPTS)`` so that TESTOPTS can actually override things
[3:07pm] mars: gary_poster, I treat TESTOPTS like CFLAGS.  VERBOSITY is independent of the two.
[3:08pm] mars: gary_poster, ok, interesting.  That woudl have come up regardless if I used VERBOSITY or TESTOPTS.
[3:09pm] gary_poster: yeah, calling it TESTOPTS is more correct, and makes the possible problem clearer.
[3:09pm] gary_poster: mars, I'll approve, simply asking that you consider my question...possibly later. 
[3:09pm] mars: +1

review: Approve

« Back to merge proposal