----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/834/#review1475 -----------------------------------------------------------
I probably got more into the details than you're looking for at this point, but I don't have much to say about the larger aspects. I'm not completely sure where this is going as far as the big picture, but I don't see anything to be too worried about yet. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1914> You forgot power. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1915> It's currently quick and long which don't quite go together. quick, medium, and fast almost go together, but quick and fast are the same thing. Also, we're talking about running time primarily and not speed. I think what makes the most sense is short, medium, and long. Also, you might want to add something for build type (debug, opt, fast). new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1916> I'm sure there's a reason, but why can't we have an __init__? new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1917> We want to be able to have tests in other directories too, right? Maybe not in v1? new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1918> I would really like to see us move away from this hardcoded path since it makes our simulation scripts brittle. This is inherited, though, and somewhat orthogonal. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1919> Yes. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1920> What's the difference between <test> and <canonical_name>? new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1921> This feels like a hack. At the very least this name shouldn't be hardcoded, and even better this case should be handled by a wrapper function that prestrips the file contents. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1923> It would be better to have an explicit list than to just grab everything and filter things out. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1922> Hardcoding stats.txt isn't great. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1924> It sounds like you realize this is a limitation, but we'll want to only generate test objects for tests that will work, have reference outputs, run on arch == whatever, etc. new_tests/conftest.py <http://reviews.m5sim.org/r/834/#comment1925> This help text is inconsistent since it's worded as a question and the others are statements. Also, two of them say they can be specified multiple times and two don't. I'm suspecting there isn't actually any difference. This particular option would theoretically accept multiple selections, right? new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1926> It's a silly nit, but could you space this out? Otherwise the comment looks like it goes with the imports. It obviously doesn't if you look at it for a few seconds, but you have to look at it for a few seconds. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1927> Same comment as above. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1928> I see what you're doing here and your names make sense when you see what's going on, but the similarity of test_root and tests_root is a little confusing at first. I think there's an os or sys function to split a path into its components. Maybe you could do that and get a slice of it like this [:-1]? I think that would do it. Not a big deal. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1929> You dropped an l in call. Also, won't this not work if it's not at the end? regression_test hasn't been defined yet, and I believe the if will happen right away, not once you get to the end of the file. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1930> Why not use curly brace syntax? new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1931> space in xrange new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1932> Space after the comma? I'm not sure about that one. Without parenthesis, spacing it out makes it less obvious that you're assigning to all that at once, although it's still easy to tell. Maybe you just need parenthesis. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1933> This looks like a job for opt_parse, or whatever it's called. new_tests/hello_world/test_hello.py <http://reviews.m5sim.org/r/834/#comment1934> This may not be a good scheme because it forces you to have a particular set of categories that mean particular things. What if I don't have an arch, or cpu, or use gem5 memory, or... I'm not sure exactly what you're going for here so I don't have any suggestions. - Gabe On 2011-08-30 10:48:42, Ali Saidi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/834/ > ----------------------------------------------------------- > > (Updated 2011-08-30 10:48:42) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and > Nathan Binkert. > > > Summary > ------- > > Regressions: Start of new regression system > > This code is to start a discussion around how to implement the next > regression system for gem5. Attached is a simple example that doesn't > run gem5 yet, but is getting closer. The big question here is how to > build up configuration files to hopefully minimize duplication of code. > > > Diffs > ----- > > new_tests/conftest.py PRE-CREATION > new_tests/hello_world/test_hello.py PRE-CREATION > > Diff: http://reviews.m5sim.org/r/834/diff > > > Testing > ------- > > > Thanks, > > Ali > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
