Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added /etc/ci/chaos_monkey_test.py. #10899

Merged
merged 1 commit into from Apr 30, 2016
Merged

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Apr 28, 2016

Add a test to /etc/ci which runs a subset of test-wpt with --random-pipeline-failure-probability=0.2, and checks to make sure that there's no CRASH reports, so the constellation survived the experience, even if a lot of tests failed.

IRC conversation at http://logs.glob.uno/?c=mozilla%23servo&s=27+Apr+2016&e=27+Apr+2016#c416510

Fixes #10568.

r? @aneeshusa


This change is Reviewable

@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

Fixes #10568


for line in TEST_RESULTS.stdout:
report = json.loads(line.decode('utf-8'))
if report.get("action", None) == "process_output":

This comment has been minimized.

Copy link
@frewsxcv

frewsxcv Apr 28, 2016

Member

The default value for dict.get is None, so explicitly specifying isn't necessary

TEST_CRASHES = TEST_CRASHES or (status == "CRASH")

if TEST_CRASHES:
exit(1)

This comment has been minimized.

Copy link
@frewsxcv

frewsxcv Apr 28, 2016

Member

You probably want sys.exit

@asajeffrey asajeffrey force-pushed the asajeffrey:chaos-monkey-ci branch from dfafabc to 118126f Apr 28, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

Fixed and squashed. You can tell my python is a bit rusty.

from subprocess import Popen, PIPE


def is_crash(report):

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

This method isn't being used, so it can be removed.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 28, 2016

Author Member

Fixed.


TEST_RESULTS = Popen(TEST_CMD, stdout=PIPE)
TEST_CRASHES = False
TEST_STDOUT = {}

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

None of these three are constants, so they should be in lowercase camel_case.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 28, 2016

Author Member

Fixed.

# option. This file may not be copied, modified, or distributed
# except according to those terms.

import json

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

Let's add from __future__ import absolute_import, print_function above this (with an empty line between that and import json).

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 28, 2016

Author Member

Done.

for line in TEST_RESULTS.stdout:
report = json.loads(line.decode('utf-8'))
if report.get("action") == "process_output":
print(report.get("thread") + " - " + report.get("data"))

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

Let's use str.format to make these nicer:

print("{} - {}".format(report.get('thread'), report.get('data')))

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 28, 2016

Author Member

Done.

print(report.get("thread") + " - " + report.get("data"))
status = report.get("status")
if status:
print(report.get("thread") + " - " + status + " - " + report.get("test"))

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

Ditto about using str.format.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 28, 2016

Author Member

Done.

@aneeshusa
Copy link
Member

aneeshusa commented Apr 28, 2016

Which outputs of this do you need? We can gate on the exit code, and Buildbot will automatically pick up on anything we print() to stdout. If you want to capture the chaos-monkey.log, we'll need to make that explicit when adding it to steps.yml (or send it to stderr, which should get picked up automatically).

I looked at the output and there's quite a bit of it - do we only want to log the CRASHes to output, or is having the rest of it useful as well?

status = report.get("status")
if status:
print(report.get("thread") + " - " + status + " - " + report.get("test"))
TEST_CRASHES = TEST_CRASHES or (status == "CRASH")

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

style nit: I find this easier to read, but up to you:

if status == "CRASH":
    TEST_CRASHES = True
@aneeshusa
Copy link
Member

aneeshusa commented Apr 28, 2016

Will we ever have both action == 'process_output' and a status? Not sure if you want to avoid printing two lines in that case.

@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

IRC chat about outputs: http://logs.glob.uno/?c=mozilla%23servo&s=28+Apr+2016&e=28+Apr+2016#c417879

TL;DR: don't capture the whole log.

@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

action and status are exclusive, even if we do get a line with both, it's worth printing out both lots of info.

@asajeffrey asajeffrey force-pushed the asajeffrey:chaos-monkey-ci branch from 118126f to ee8a4ad Apr 28, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

Fixed and squashed.

@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 28, 2016

I also increased the probability of pipeline closure to 0.1, to make sure that some crashes happen! This magic number may need played with.

# returned by the test command (which is why we can't use check_output).

test_results = Popen(TEST_CMD, stdout=PIPE)
test_crashes = False

This comment has been minimized.

Copy link
@aneeshusa

aneeshusa Apr 28, 2016

Member

Quick nit: Let's change this to any_crashes because if any_crashes reads better.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Apr 29, 2016

Author Member

Sure. I'll do this tomorrow, when I'm at a computer.

@asajeffrey asajeffrey force-pushed the asajeffrey:chaos-monkey-ci branch from ee8a4ad to ac03728 Apr 29, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 29, 2016

Fixed and squashed.

@aneeshusa
Copy link
Member

aneeshusa commented Apr 29, 2016

@bors-servo r+

We should file a follow-up issue for gating on this in the CI.

@bors-servo
Copy link
Contributor

bors-servo commented Apr 29, 2016

📌 Commit ac03728 has been approved by aneeshusa

@asajeffrey
Copy link
Member Author

asajeffrey commented Apr 29, 2016

Filed #10927

@bors-servo
Copy link
Contributor

bors-servo commented Apr 29, 2016

Testing commit ac03728 with merge a7a8eba...

bors-servo added a commit that referenced this pull request Apr 29, 2016
Added /etc/ci/chaos_monkey_test.py.

Add a test to `/etc/ci` which runs a subset of `test-wpt` with `--random-pipeline-failure-probability=0.2`, and checks to make sure that there's no `CRASH` reports, so the constellation survived the experience, even if a lot of tests failed.

IRC conversation at http://logs.glob.uno/?c=mozilla%23servo&s=27+Apr+2016&e=27+Apr+2016#c416510

Fixes #10568.

r? @aneeshusa

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10899)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 30, 2016

@bors-servo bors-servo merged commit ac03728 into servo:master Apr 30, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.