Sorry, forgot to reply all, one of these days I'll get used to doing that.
Brock
--- Begin Message ---
Danek Duvall wrote:
Sorry for the late review.
On Thu, May 29, 2008 at 03:38:54PM -0700, Brock Pytlik wrote:
webrev at: http://cr.opensolaris.org/~bpytlik/ips-fix-2085/
- There's no evaluation in the bug. What's the actual cause of the
problem, and why do we need to move to generating the files at run-time
instead of using the static ones in the gate? Could sys.path[0] be
used to find the static files?
I didn't realize I was supposed to put evaluation back into the bug. Are
there a couple of bugs that have a good example of this so that I can
see what kind of information should be provided?
I'll add this to the bug file, but I'll pass it along here as well. If
you'd let me know if this is sort of the information you'd look to have
in the bug I'd appreciate it.
The problem is that, when run from cli-complete, t_actions.py doesn't
know where to look for the files. Relying on a relative directory
relationship between cli-complete.py and t_actions.py seemed like a
brittle approach to me as did relying on either being run from a
particular directory. We may well reorganize the test suite in the
future with more hierarchy, and that change would have to be reflected
in the code, and the movement of the test program and data together.
Further, I would argue that, where possible, the tests should be self
contained, both in their execution and their code. What I mean by that
is all the code for running a test should be in the file for the test
itself or the things it imports (which are easy to track) and that
having paths hidden within the code makes it more likely that things
will break when the code is reorganized. Also, when executing, the test
should create a single directory (unavoidable) and do all its work
inside that directory, not touching the rest of /tmp or the rest of the
file system.
I don't see a reason not to generate the files at run-time. The content
of the files is easy to determine from inspection of the test case, and
it avoids the problem of having two tests using the same data files with
different expectations. For example, a person writing test A makes the
data files to test something specific. Later, a person writing test B
changes the files to test something specific, but in doing so, changes
the file to make test A meaningless. Since the dependency for test A
isn't obvious, it would be easy for the second person to miss that they
had broken test A.
- spaces after commas
fixed
- The webrev doesn't indicate that you're getting rid of the files in the
testdata directory (the checked-in ones); presumably they're not needed any
more?
good point, I'll pull them out
- The string + variable + string constructions in the manifests are,
IMHO, difficult to read. I'd use %s expandos instead.
that's funny cause I find the %s expandos very difficult to read (ie, I
purposefully avoid using them when writing code), but if that's the
standard style we're using as a group, then I'll switch
- You don't seem to do anything with "empty". Does that not cause
problems, too?
I'm not sure what you mean. The empty file was in fact, empty. It gets
created in the set up that loops over misc_files and has no data in it.
- Ditch the ident string in the ftpusers file.
Ok, it was in the original file, so I wanted to duplicate them exactly
(I wasn't sure exactly which lines were critical for the test).
- line 139ff: Not a requirement, but I'd probably use eval here to reduce
the elif cluster to a single line.
Can you explain what you mean by using eval? I don't see what you mean.
Danek
--- End Message ---
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
