----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31645/#review79271 -----------------------------------------------------------
support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128511> not sure what the 'accepted standard' is here, but I'd suggest we make this code Python 3-ready: ``` from __future__ import print_function ... print('foo bar') ``` Also, I would much prefer the use of `string.format()` to the use of `%` ``` print("Run {}, output: {}".format(args, outfile)) ``` Finally (but it really is nit-picking here): you are blindly printing args, if they include a password they may show up in logs, etc. support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128513> ummm... not really a `sleep` method this one, is it? :) also, I'd document that we return a `bool` (and what the meaning is) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128512> can you make poll_time a configurable arg for this method? ``` def sleep(self, seconds, poll_time=0.1) ``` (or it could even be part of the class `__init__()`) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128515> umm... shouldn't you call this one with Python's intuitive and friendly way to call the super-class? ``` super(Slave, self).__init__(...) ``` (not sure how you make this Python3-friendly for the future, but that's probably another story) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128514> why not adding a `framework` arg: ``` def __init__(self, path, credfile, framework='test-framework') ``` support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128516> not sure I completely agree with a function that returns objects of different types depending on the codepath... sure, this works, to the extent that one can do: ``` if not version(path): # blow up ``` but I'd prefer a `None` returned (same effect as above) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128517> are you trying to trim the `\n` here? (if not, what? sorry, can't quite recall what p.communicate() returns) how about `output.strip()`? (that is, assuming that's what this is doing) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128518> the "pythonic" way of doing this would be to ``` def parse_args(): ... return parser.parse_args() def main(config): .... if __name__ == '__main__': config = parse_args() main(config) ``` Also please split the argparsing into its own `parse_args()` method and just pass a `config` to main() support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128519> what is this? support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128520> I would very *strongly* advocate the use of Jinja2 templates here ;) support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128521> if you use `string.format()` this becomes a much more pleasant: ``` "... #1 | {version}\t | .... ...".format(version=prev_version) ``` support/test-upgrade.py <https://reviews.apache.org/r/31645/#comment128523> DRY how about factoring this out to a common method? (or, god forbid, a decorator) This looks good - but it looks to me that the 'main' part could be written so as to be more data-driven (such as reading a JSON file and executing each test case there described). If you want, I'd be happy to have a go at it. - Marco Massenzio On March 2, 2015, 11:44 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31645/ > ----------------------------------------------------------- > > (Updated March 2, 2015, 11:44 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-2372 > https://issues.apache.org/jira/browse/MESOS-2372 > > > Repository: mesos > > > Description > ------- > > Upgrade path test script runs previous and new framework versions (i.e. > different versions of test framework+libmesos) against different versions of > mesos slave and master. We can hopefully generate the upgrade paths we want > to test systematically (applying some combinatorics) and cover those in this > script. > > > Diffs > ----- > > support/test-upgrade.py PRE-CREATION > > Diff: https://reviews.apache.org/r/31645/diff/ > > > Testing > ------- > > The output from running the script against 0.21.0 and HEAD (0.23.0): > > $ python ./support/run-upgrade.py --prev=../mesos/build-0.21.0/ --next=build > Running upgrade test from mesos 0.21.0 to mesos 0.23.0 > +--------------+----------------+----------------+---------------+ > | Test case | Framework | Master | Slave | > +--------------+----------------+----------------+---------------+ > | #1 | mesos 0.21.0 | mesos 0.21.0 | mesos 0.21.0 | > | #2 (live) | mesos 0.21.0 | mesos 0.21.0 | mesos 0.23.0 | > | #3 | mesos 0.23.0 | mesos 0.21.0 | mesos 0.23.0 | > | #4 | mesos 0.23.0 | mesos 0.23.0 | mesos 0.23.0 | > +--------------+----------------+----------------+---------------+ > > NOTE: live denotes that master process keeps running from previous case. > > > Test case 1 (Run of previous setup) > ##### Starting mesos 0.21.0 master ##### > Run ['../mesos/build-0.21.0/bin/mesos-master.sh', '--ip=127.0.0.1', > '--work_dir=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmp2UMcJv', > '--credentials=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T > /tmpEMpQQd'], output: > /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpEXE5wH > ##### Starting mesos 0.21.0 slave ##### > Run ['../mesos/build-0.21.0/bin/mesos-slave.sh', '--master=localhost:5050', > '--credential=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpEMpQQd'], > output: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000 > gn/T/tmpnGiJfB > ##### Starting mesos 0.21.0 framework ##### > Waiting for mesos 0.21.0 framework to complete (10 sec max)... > Run ['../mesos/build-0.21.0/src/test-framework', '--master=localhost:5050'], > output: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmp5cfI3A > > Test case 2 (Upgrade of slave) > ##### Stopping mesos 0.21.0 slave ##### > ##### Starting mesos 0.23.0 slave ##### > Run ['build/bin/mesos-slave.sh', '--master=localhost:5050', > '--credential=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpEMpQQd'], > output: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpj9sM2K > ##### Starting mesos 0.21.0 framework ##### > Waiting for mesos 0.21.0 framework to complete (10 sec max)... > Run ['../mesos/build-0.21.0/src/test-framework', '--master=localhost:5050'], > output: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpltfC_p > > Test case 3 (Upgrade framework) > ##### Starting mesos 0.23.0 framework ##### > Waiting for mesos 0.23.0 framework to complete (10 sec max)... > Run ['build/src/test-framework', '--master=localhost:5050'], output: > /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpJVWJjW > > Test case 4 (Run of next setup) > ##### Stopping mesos 0.23.0 slave #### > ##### Stopping mesos 0.21.0 slave #### > Run ['build/bin/mesos-master.sh', '--ip=127.0.0.1', > '--work_dir=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpALCpmO', > '--credentials=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpEMpQQd'], > o$tput: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmp6oLkTt > ##### Starting mesos 0.23.0 slave ##### > Run ['build/bin/mesos-slave.sh', '--master=localhost:5050', > '--credential=/var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpEMpQQd'], > output: /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpYOCLCM > ##### Starting mesos 0.23.0 framework ##### > Waiting for mesos 0.23.0 framework to complete (10 sec max)... > Run ['build/src/test-framework', '--master=localhost:5050'], output: > /var/folders/y3/w04yjljd5gbcvbvxvd5bhmp40000gn/T/tmpCVjtu3 > > > Thanks, > > Niklas Nielsen > >
