Awight has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/345221 )
Change subject: Log, don't print
......................................................................
Log, don't print
Change-Id: I35f263e6bba7fe4bfa16e5bfba57e39cf436cd18
---
M processcontrol/job_wrapper.py
M processcontrol/lock.py
M test-requirements.txt
M tests/test_job_wrapper.py
4 files changed, 38 insertions(+), 51 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/wikimedia/fundraising/process-control
refs/changes/21/345221/1
diff --git a/processcontrol/job_wrapper.py b/processcontrol/job_wrapper.py
index 4d3892e..a2b8838 100644
--- a/processcontrol/job_wrapper.py
+++ b/processcontrol/job_wrapper.py
@@ -1,10 +1,8 @@
-from __future__ import print_function
import datetime
import glob
import os
import shlex
import subprocess
-import sys
import threading
from . import config
@@ -79,21 +77,21 @@
def fail_exitcode(self, return_code):
message = "Job {name} failed with code {code}".format(name=self.name,
code=return_code)
- print(message, file=sys.stderr)
+ config.log.error(message)
# TODO: Prevent future jobs according to config.
self.mailer.fail_mail(message)
def fail_has_stderr(self, stderr_data):
message = "Job {name} printed things to stderr:".format(name=self.name)
- print(message, file=sys.stderr)
+ config.log.error(message)
body = stderr_data.decode("utf-8")
- print(body, file=sys.stderr)
+ config.log.error(body)
self.mailer.fail_mail(message, body)
def fail_timeout(self):
self.process.kill()
message = "Job {name} timed out after {timeout}
seconds".format(name=self.name, timeout=self.timeout)
- print(message, file=sys.stderr)
+ config.log.error(message)
self.mailer.fail_mail(message)
# FIXME: Job will return SIGKILL now, fail_exitcode should ignore that
signal now?
@@ -114,7 +112,7 @@
"{name} ({pid}), started at {time}\n"
"-----------\n"
).format(name=self.name, pid=self.process.pid,
time=self.start_time.isoformat())
- print(header, file=out)
+ out.write(header)
if len(stdout_data) == 0:
buf = "* No output *\n"
@@ -124,10 +122,10 @@
if len(stderr_data) > 0:
header = (
- "-----------\n",
- "Even worse, the job emitted errors:\n",
- "-----------\n",
+ "-----------\n"
+ "Even worse, the job emitted errors:\n"
+ "-----------\n"
)
- print(header, file=out)
+ out.write(header)
out.write(stderr_data.decode("utf-8"))
diff --git a/processcontrol/lock.py b/processcontrol/lock.py
index 6af0bb5..24df5a2 100644
--- a/processcontrol/lock.py
+++ b/processcontrol/lock.py
@@ -3,9 +3,8 @@
Self-corrects stale locks unless "failopen" is True.
'''
-from __future__ import print_function
+import config
import os
-import sys
lockfile = None
@@ -15,7 +14,7 @@
filename = "/tmp/{name}.lock".format(name=job_tag)
if os.path.exists(filename):
- print("Lockfile found!", file=sys.stderr)
+ config.log.error("Lockfile found!")
with open(filename, "r") as f:
pid = None
try:
@@ -24,7 +23,7 @@
pass
if not pid:
- print("Invalid lockfile contents.", file=sys.stderr)
+ config.log.error("Invalid lockfile contents.")
else:
try:
os.getpgid(pid)
@@ -32,11 +31,12 @@
except OSError:
if failopen:
raise LockError("Aborting until stale lockfile is
investigated: {path}".format(path=filename))
- print("Lockfile is stale.", file=sys.stderr)
- print("Removing old lockfile.", file=sys.stderr)
+ config.log.error("Lockfile is stale.")
+ config.log.error("Removing old lockfile.")
os.unlink(filename)
with open(filename, "w") as f:
+ config.log.info("Writing lockfile.")
f.write(str(os.getpid()))
global lockfile
@@ -46,6 +46,7 @@
def end():
global lockfile
if lockfile and os.path.exists(lockfile):
+ config.log.info("Clearing lockfile.")
os.unlink(lockfile)
else:
raise LockError("Already unlocked!")
diff --git a/test-requirements.txt b/test-requirements.txt
index fb6d7c7..d503db3 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -1,5 +1,5 @@
-iocapture
mock
nose
-pytest
nose-cov
+pytest
+testfixtures
diff --git a/tests/test_job_wrapper.py b/tests/test_job_wrapper.py
index e85500a..8490826 100644
--- a/tests/test_job_wrapper.py
+++ b/tests/test_job_wrapper.py
@@ -1,8 +1,8 @@
import glob
-import iocapture
import mock
import nose
import os
+import testfixtures
from processcontrol import job_wrapper
@@ -30,21 +30,18 @@
def test_success():
- with iocapture.capture() as captured:
- run_job("successful.yaml")
+ run_job("successful.yaml")
- assert captured.stdout == ""
- assert captured.stderr == ""
+ # TODO: assert more
@mock.patch("smtplib.SMTP")
def test_return_code(MockSmtp):
- expected = "Job False job failed with code 1\n"
- with iocapture.capture() as captured:
+ with testfixtures.LogCapture() as caplog:
run_job("return_code.yaml")
- assert captured.stdout == ""
- assert captured.stderr == expected
+ loglines = caplog.actual()
+ assert ("root", "ERROR", "Job False job failed with code 1") in
loglines
MockSmtp().sendmail.assert_called_once()
@@ -53,29 +50,25 @@
@nose.tools.timed(2)
@mock.patch("smtplib.SMTP")
def test_timeout(MockSmtp):
- with iocapture.capture() as captured:
+ with testfixtures.LogCapture() as caplog:
run_job("timeout.yaml")
- assert captured.stdout == ""
- assert captured.stderr == (
- "Job Timing out job timed out after 0.1 seconds\n"
- "Job Timing out job failed with code -9\n"
- )
+ loglines = caplog.actual()
+ assert ("root", "ERROR", "Job Timing out job timed out after 0.1
seconds") in loglines
+ assert ("root", "ERROR", "Job Timing out job failed with code -9") in
loglines
MockSmtp().sendmail.assert_called_once()
@mock.patch("smtplib.SMTP")
def test_stderr(MockSmtp):
- with iocapture.capture() as captured:
+ with testfixtures.LogCapture() as caplog:
run_job("errors.yaml")
- assert captured.stdout == ""
- assert captured.stderr == (
- "Job Bad grep job printed things to stderr:\n"
- "grep: Invalid regular expression\n\n"
- "Job Bad grep job failed with code 2\n"
- )
+ loglines = list(caplog.actual())
+ assert ("root", "ERROR", "Job Bad grep job printed things to stderr:")
in loglines
+ assert ("root", "ERROR", "grep: Invalid regular expression\n") in
loglines
+ assert ("root", "ERROR", "Job Bad grep job failed with code 2") in
loglines
MockSmtp().sendmail.assert_called_once()
@@ -90,8 +83,8 @@
contents = open(path, "r").read()
lines = contents.split("\n")
- assert len(lines) == 6
- assert lines[4] == "/bin/bash"
+ assert len(lines) == 5
+ assert "/bin/bash" in lines
os.unlink(path)
@@ -105,15 +98,10 @@
path = log_files[-1]
contents = open(path, "r").read()
lines = contents.split("\n")
- print(lines)
- assert len(lines) == 7
+ assert len(lines) == 6
- dumped_env = sorted(lines[4:6])
- expected = [
- "foo1=bar",
- "foo2=rebar",
- ]
- assert expected == dumped_env
+ assert "foo1=bar" in lines
+ assert "foo2=rebar" in lines
os.unlink(path)
--
To view, visit https://gerrit.wikimedia.org/r/345221
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I35f263e6bba7fe4bfa16e5bfba57e39cf436cd18
Gerrit-PatchSet: 1
Gerrit-Project: wikimedia/fundraising/process-control
Gerrit-Branch: master
Gerrit-Owner: Awight <awi...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
