Re: Review Request 60235: Linted support/test-upgrade.py.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/#review182608
---


Ship it!




Only some slight changes in error messages if/when the test framework fails.  
Not a big deal though, as we can glean the version of the agent/master from the 
other printed messages.

- Joseph Wu


On July 29, 2017, 2:03 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated July 29, 2017, 2:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/3/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/
---

(Updated July 29, 2017, 9:03 a.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Resolved issue.


Bugs: MESOS-6390
https://issues.apache.org/jira/browse/MESOS-6390


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 


Diff: https://reviews.apache.org/r/60235/diff/3/

Changes: https://reviews.apache.org/r/60235/diff/2-3/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

> On July 17, 2017, 11:30 p.m., Joseph Wu wrote:
> > support/test-upgrade.py
> > Lines 114-115 (original), 142-147 (patched)
> > 
> >
> > This refactoring doesn't retain the original logic.
> > 
> > The script does the following:
> > 
> > 1) Launch the Master/Agent/Framework of version A.
> > 2) Kill Agent A, kill Master A, start Master B, start Agent A, start 
> > Framework A.  The agent is only restarted here due to how the 
> > StandaloneMasterDetector is implemented.
> > 3) Kill Agent A, start Agent B, start Framework A.
> > 4) Start Framework B.
> > 
> > This refactoring will cause the master to be killed twice more than the 
> > original and the agent to be killed once more.
> > 
> > I think it would be fine to make 4 new methods for each of the test 
> > steps (start_cluster, upgrade_master, upgrade_agent, upgrade_framework).

I should have documented the change, this is the result of a discussion with 
Benjamin about the fact that these are not test cases if they are not 
independent. As it is not linting anymore I am thinking abour renaming the 
review request "Refactored support/test-upgrade.py.", would you rather suggest 
a linting in that review request with an other one doing the change in the 
logic?


- Armand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/#review180765
---


On June 26, 2017, 3:48 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 26, 2017, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/#review180765
---




support/test-upgrade.py
Lines 114-115 (original), 142-147 (patched)


This refactoring doesn't retain the original logic.

The script does the following:

1) Launch the Master/Agent/Framework of version A.
2) Kill Agent A, kill Master A, start Master B, start Agent A, start 
Framework A.  The agent is only restarted here due to how the 
StandaloneMasterDetector is implemented.
3) Kill Agent A, start Agent B, start Framework A.
4) Start Framework B.

This refactoring will cause the master to be killed twice more than the 
original and the agent to be killed once more.

I think it would be fine to make 4 new methods for each of the test steps 
(start_cluster, upgrade_master, upgrade_agent, upgrade_framework).


- Joseph Wu


On June 26, 2017, 8:48 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 26, 2017, 8:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/2/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/
---

(Updated June 26, 2017, 3:48 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Resolved issues and new function `test_case` to make the code simpler.


Bugs: MESOS-6390
https://issues.apache.org/jira/browse/MESOS-6390


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs (updated)
-

  support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 


Diff: https://reviews.apache.org/r/60235/diff/2/

Changes: https://reviews.apache.org/r/60235/diff/1-2/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet

Re: Review Request 60235: Linted support/test-upgrade.py.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/#review178877
---


Fix it, then Ship it!





support/test-upgrade.py
Lines 34 (patched)


Could we add a one-line, PEP257-conformant summary here, e.g.,

"""
Helper class to keep track of process lifecycles.

This class allows to i.e., start processes, capture their output,
and checking their liveness during delays/sleep.
"""

Also, add a blank line before declaring the first method.



support/test-upgrade.py
Lines 49 (patched)


Let's add a PEP257-conformant summary line, e.g.,

"""
Poll the process for the specified number of seconds.

We return the process's return value if it ends during that
time. Returns `True` if the process is still running after
that time period.
"""



support/test-upgrade.py
Line 48 (original), 69 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Line 60 (original), 79 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Line 75 (original), 91 (patched)


Add an empty line before declaring the first method.



support/test-upgrade.py
Lines 102-103 (original), 116-118 (patched)


Let's fit this onto a single line, e.g.,

"""Convenience function to get the Mesos version from built 
executables."""



support/test-upgrade.py
Line 119 (original), 134 (patched)


Let's add a one-line, PEP257-conformant summary, e.g.,

Main function.



support/test-upgrade.py
Line 138 (original), 158 (patched)


I think explicitly converting this `Boolean`s to `Boolean`s is pretty 
verbose, how about

if not prev_version or not next_version:



support/test-upgrade.py
Line 176 (original), 196 (patched)


Let's rely on the thruthiness of `sleep`'s return value, e.g.,

if not prev_master.sleep(0.5):



support/test-upgrade.py
Line 183 (original), 203 (patched)


Let's rely on the thruthiness of `sleep`'s return value, e.g.,

if not prev_agent.sleep(0.5):


- Benjamin Bannier


On June 20, 2017, 12:16 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60235/
> ---
> 
> (Updated June 20, 2017, 12:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-6390
> https://issues.apache.org/jira/browse/MESOS-6390
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will allow us to use PyLint on the
> entire support directory in the future.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/60235/diff/1/
> 
> 
> Testing
> ---
> 
> Added `support` to `source_dirs` in the PyLinter defined
> in `mesos-style.py`. Linted until only acceptable errors
> where displayed (e.g. due to the name of the file containing
> a dash instead of an underscore).
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 60235: Linted support/test-upgrade.py.

[Armand Grillet]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60235/
---

Review request for mesos, Benjamin Bannier and Joseph Wu.


Bugs: MESOS-6390
https://issues.apache.org/jira/browse/MESOS-6390


Repository: mesos


Description
---

This will allow us to use PyLint on the
entire support directory in the future.


Diffs
-

  support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 


Diff: https://reviews.apache.org/r/60235/diff/1/


Testing
---

Added `support` to `source_dirs` in the PyLinter defined
in `mesos-style.py`. Linted until only acceptable errors
where displayed (e.g. due to the name of the file containing
a dash instead of an underscore).


Thanks,

Armand Grillet