Differential D70

changing resultsdb directive code to create and complete resultsdb job during the reporting process. Fixes T132
ClosedPublic
Actions

Authored by tflink on Apr 24 2014, 5:07 PM.

Details

Reviewers
kparal
mkrizek
jskladan
Maniphest Tasks
T132: Create resultsdb job in resultsdb directive for now
Commits
rLTRN16c59915bc8f: changing resultsdb directive code to create and complete resultsdb job during…
Summary

adds functionality to resultsdb directive so that the job is created during reporting.

Test Plan

new unit tests created, tested locally with dev instance of resultsdb

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
tflink updated this revision.Apr 24 2014, 6:49 PM
  • updating docstrings to reflect actual TAP output
  • fixing docstrings to be valid and complete
tflink updated this revision.Apr 24 2014, 6:49 PM
  • fixing docstrings to be valid and complete
mkrizek added inline comments.Apr 25 2014, 2:21 PM
libtaskotron/directives/resultsdb_directive.py
103

Why this print?

113

Shouldn't this check be before creating the job? If testcase doesn't exist, create_job will fail and this check wouldn't be run at all, or am missing something?

tflink updated this revision.Apr 25 2014, 2:28 PM
  • removing stray print that shouldn't be in review
tflink added inline comments.Apr 25 2014, 3:13 PM
libtaskotron/directives/resultsdb_directive.py
103

Good question, it probably shouldn't be there

113

testcase isn't used until the results are reported and thus, doesn't need to exist before the job is created.

kparal accepted this revision.Apr 25 2014, 3:16 PM

I don't have anything but a few nitpicks. I'm not really able to verify that it's semantically correct, @jskladan should be able to confirm that. But if we break something, I assume we'll learn it soon enough :)

libtaskotron/directives/resultsdb_directive.py
55–56

I'm not really fond of matching error messages. If we need to distinguish different cases, let's create a child exception and than compare by its class.

If we need to push this ASAP, let's create a ticket for it instead and add FIXME to the code.

59

IIUIC and we intend to patch this, please prepend with FIXME.

70–72

Again, please prepend with FIXME or TODO. Some editors nicely highlight it, and it's easy to grep for these words in console.

106

It's better to print e, because e.message might be empty. I don't know how it works in this particular case.

tflink closed this revision.Apr 25 2014, 3:27 PM

Closed by commit rLTRN16c59915bc8f (authored by @tflink).

Revision Contents

Diff 201

View Options

libtaskotron/directives/resultsdb_directive.py

Show All 35 Lines32class ResultsdbDirective(BaseDirective):
36 """ 36​ """
37 37​
38 def __init__(self, resultsdb = None): 38​ def __init__(self, resultsdb = None):
39 self.resultsdb = resultsdb 39​ self.resultsdb = resultsdb
40 if self.resultsdb is None: 40​ if self.resultsdb is None:
41 Config = config.get_config() 41​ Config = config.get_config()
42 self.resultsdb = resultsdb_api.ResultsDBapi(Config.resultsdb_server) 42​ self.resultsdb = resultsdb_api.ResultsDBapi(Config.resultsdb_server)
43 43​
44​ def ensure_testcase_exists(self, name):
45​ """ Make sure that the testcase exists in resultsdb, otherwise create
46​ the testcase using a dummy url as a reference
47​
48​ :param str name: name of the testcase to check for
49​ """
50​
51​ try:
52​ testcase = self.resultsdb.get_testcase(name)
53​ return
54​ except resultsdb_api.ResultsDBapiException, e:
55​ if e.message != 'Testcase not found':
56​ raise e
kparalUnsubmitted
Not Done

I'm not really fond of matching error messages. If we need to distinguish different cases, let's create a child exception and than compare by its class.

If we need to push this ASAP, let's create a ticket for it instead and add FIXME to the code.

57​
58​ # since the testcase doesn't exist, create it with a dummy value for url
59​ # it can be updated later when it's not blocking results reporting
kparalUnsubmitted
Not Done

IIUIC and we intend to patch this, please prepend with FIXME.

60​ dummyurl = 'http://faketestcasesRus.com/%s' % name
61​ self.resultsdb.create_testcase(name, dummyurl)
62​
63​ def create_resultsdb_job(self, name, refurl=None):
64​ """ Create a job in resultsdb for reporting results against
65​
66​ :param str name: name of job to report against
67​ :returns: dict of rendered json results
68​ """
69​
70​ # this is completly hacky for now since the runner doesn't have a
71​ # good way to get the actual refurl. This will be addressed in T122 and
72​ # T145
kparalUnsubmitted
Not Done

Again, please prepend with FIXME or TODO. Some editors nicely highlight it, and it's easy to grep for these words in console.

73​
74​ url = 'http://not-a-real-domain.com/noworky'
75​ if refurl is not None:
76​ url = refurl
77​
78​ return self.resultsdb.create_job(url, status='RUNNING', name=name)
79​
80​ def complete_resultsdb_job(self, jobid):
81​ """ Change the resultsdb job to a status of 'COMPLETED', indicating
82​ that the reporting is complete.
83​
84​ :param int jobid: The resultsdb jobid to be modified
85​ :returns: json representation of returned data from the resultsdb instance
86​ """
87​ return self.resultsdb.update_job(jobid, status='COMPLETED')
88​
44 def process(self, input_data, env_data): 89​ def process(self, input_data, env_data):
45 90​
46 Config = config.get_config() 91​ Config = config.get_config()
47 if not (Config.reporting_enabled and Config.report_to_resultsdb): 92​ if not (Config.reporting_enabled and Config.report_to_resultsdb):
48 log.info("Reporting to ResultsDB is disabled.") 93​ log.info("Reporting to ResultsDB is disabled.")
49 return 94​ return
50 95​
51 #TODO: Replace with proper location and adjust exc. text 96​ #TODO: Replace with proper location and adjust exc. text
52 if 'resultsdb_job_id' not in env_data or 'checkname' not in env_data: 97​ # we're creating the jobid in the directive for now, so adjust the check
98​
99​ if 'checkname' not in env_data:
53 raise TaskotronDirectiveError("The resultsdb directive requires "\ 100​ raise TaskotronDirectiveError("The resultsdb directive requires "\
54 "resultsdb_job_id and checkname.") 101​ "resultsdb_job_id and checkname.")
55 102​
56 try: 103​ try:
mkrizekUnsubmitted
Not Done

Why this print?

tflinkAuthorUnsubmitted
Not Done

Good question, it probably shouldn't be there

57 check_details = check.import_TAP(input_data['results']) 104​ check_details = check.import_TAP(input_data['results'])
58 except TaskotronValueError as e: 105​ except TaskotronValueError as e:
59 raise TaskotronDirectiveError("Failed to load 'results': %s" % e.message) 106​ raise TaskotronDirectiveError("Failed to load 'results': %s" % e.message)
kparalUnsubmitted
Not Done

It's better to print e, because e.message might be empty. I don't know how it works in this particular case.

60 107​
108​ # for now, we're creating the resultsdb job at reporting time
109​ # the job needs to be 'RUNNING' in order to append any results
110​ jobdata = self.create_resultsdb_job(env_data['checkname'])
111​
112​ # check to make sure that the testcase/checkname exists, create it if it doesn't
113​ self.ensure_testcase_exists(env_data['checkname'])
mkrizekUnsubmitted
Not Done

Shouldn't this check be before creating the job? If testcase doesn't exist, create_job will fail and this check wouldn't be run at all, or am missing something?

tflinkAuthorUnsubmitted
Not Done

testcase isn't used until the results are reported and thus, doesn't need to exist before the job is created.

114​
61 output = [] 115​ output = []
62 116​
63 for detail in check_details: 117​ for detail in check_details:
64 try: 118​ try:
65 result = self.resultsdb.create_result( 119​ result = self.resultsdb.create_result(
66 job_id = env_data['resultsdb_job_id'], #TODO: replace with proper location 120​ job_id = jobdata['id'],
121​ #env_data['resultsdb_job_id'], #TODO: replace with proper location
67 testcase_name = env_data['checkname'], 122​ testcase_name = env_data['checkname'],
68 outcome = detail.outcome, 123​ outcome = detail.outcome,
69 summary = detail.summary or None, 124​ summary = detail.summary or None,
70 log_url = "http://example.com", #TODO: replace with proper location, 125​ log_url = "http://example.com", #TODO: replace with proper location,
71 item = detail.item, 126​ item = detail.item,
72 type = detail.report_type, 127​ type = detail.report_type,
73 **detail.keyvals) 128​ **detail.keyvals)
74 output.append(result) 129​ output.append(result)
75 130​
76 except resultsdb_api.ResultsDBapiException: 131​ except resultsdb_api.ResultsDBapiException, e:
77 log.info("Failed to store ResulsDB: `%s` `%s` `%s`", 132​ log.error(e)
133​ log.error("Failed to store ResulsDB: `%s` `%s` `%s`",
78 detail.item, env_data['checkname'], detail.outcome) 134​ detail.item, env_data['checkname'], detail.outcome)
79 135​
136​ # move job to completed
137​ self.complete_resultsdb_job(jobdata['id'])
138​
80 return output 139​ return output
81 140​
View Options

testing/test_resultsdb_directive.py

1"""Unit tests for libtaskotron/directives/resultsdb_directive.py""" 1​"""Unit tests for libtaskotron/directives/resultsdb_directive.py"""
2 2​
3import pytest 3​import pytest
4from copy import deepcopy 4​from copy import deepcopy
5from dingus import Dingus 5​from dingus import Dingus, exception_raiser
6 6​
7from libtaskotron.directives import resultsdb_directive 7​from libtaskotron.directives import resultsdb_directive
8from libtaskotron.exceptions import TaskotronDirectiveError, TaskotronValueError 8​from libtaskotron.exceptions import TaskotronDirectiveError, TaskotronValueError
9 9​
10from libtaskotron import check 10​from libtaskotron import check
11from libtaskotron import config 11​from libtaskotron import config
12 12​
13​from resultsdb_api import ResultsDBapiException
13 14​
14class TestResultsdbReport(): 15​class TestResultsdbReport():
15 16​
16 def setup_method(self, method): 17​ def setup_method(self, method):
17 '''Run this before every test invocation''' 18​ '''Run this before every test invocation'''
18 19​
20​
21​
19 self.cd = check.CheckDetail( 22​ self.cd = check.CheckDetail(
20 item = 'foo_bar', 23​ item = 'foo_bar',
21 report_type = check.ReportType.KOJI_BUILD, 24​ report_type = check.ReportType.KOJI_BUILD,
22 outcome = 'NEEDS_INSPECTION', 25​ outcome = 'NEEDS_INSPECTION',
23 summary = 'foo_bar summary', 26​ summary = 'foo_bar summary',
24 output = ["foo\nbar"], 27​ output = ["foo\nbar"],
25 keyvals = {"foo": "moo1", "bar": "moo2"}, 28​ keyvals = {"foo": "moo1", "bar": "moo2"},
26 ) 29​ )
27 30​
28 31​
29 #TODO: remove this ugly hack - it is here because of the bug in bayeux 32​ #TODO: remove this ugly hack - it is here because of the bug in bayeux
30 tap = "TAP version 13\n1..1\n%s" % check.export_TAP(self.cd) 33​ tap = "TAP version 13\n1..1\n%s" % check.export_TAP(self.cd)
31 34​
32 self.ref_input = {'results': tap} 35​ self.ref_input = {'results': tap}
33 self.ref_envdata = { 36​ self.ref_envdata = {
34 'resultsdb_job_id': 1, 37​ 'resultsdb_job_id': 1,
35 'checkname': 'test_resultsdb_report', 38​ 'checkname': 'test_resultsdb_report',
36 } 39​ }
37 40​
41​ self.ref_jobid = 1234
42​ self.ref_refurl = u'http://example.com/%s' % self.ref_envdata['checkname']
43​ self.ref_jobdata = { u'end_time': None,
44​ u'href': u'http://127.0.0.1/api/v1.0/jobs/%d' % self.ref_jobid,
45​ u'id': self.ref_jobid,
46​ u'name': self.ref_envdata['checkname'],
47​ u'ref_url': self.ref_refurl,
48​ u'results': [],
49​ u'results_count': 0,
50​ u'start_time': None,
51​ u'status': u'SCHEDULED'}
52​
38 Config = config.get_config() 53​ Config = config.get_config()
39 Config.reporting_enabled = True 54​ Config.reporting_enabled = True
40 Config.report_to_resultsdb = True 55​ Config.report_to_resultsdb = True
41 56​
42 self.stub_rdb = Dingus('resultsdb') 57​ self.stub_rdb = Dingus('resultsdb', get_testcase__returns={},
58​ create_job__returns=self.ref_jobdata)
43 self.test_rdb = resultsdb_directive.ResultsdbDirective(self.stub_rdb) 59​ self.test_rdb = resultsdb_directive.ResultsdbDirective(self.stub_rdb)
44 60​
45 61​
46 def test_config_reporting_disabled(self): 62​ def test_config_reporting_disabled(self):
47 """Checks config options can disable reporting.""" 63​ """Checks config options can disable reporting."""
48 Config = config.get_config() 64​ Config = config.get_config()
49 65​
50 for r_e, r_t_r in ((False, False), (False, True), (True, False)): 66​ for r_e, r_t_r in ((False, False), (False, True), (True, False)):
51 Config.reporting_enabled = r_e 67​ Config.reporting_enabled = r_e
52 Config.report_to_resultsdb = r_t_r 68​ Config.report_to_resultsdb = r_t_r
53 69​
54 assert self.test_rdb.process(self.ref_input, self.ref_envdata) is None 70​ assert self.test_rdb.process(self.ref_input, self.ref_envdata) is None
55 71​
56 config._config = None 72​ config._config = None
57 73​
58 def test_missing_resultsdb_job_id(self, monkeypatch): 74​ def test_missing_checkname(self):
59 """Check if missing job_id raises exception""" ​
60 with pytest.raises(TaskotronDirectiveError): ​
61 envdata = deepcopy(self.ref_envdata) ​
62 del(envdata['resultsdb_job_id']) ​
63 ​
64 self.test_rdb.process(self.ref_input, envdata) ​
65 ​
66 def test_missing_checkname(self, monkeypatch): ​
67 """Checks if missing checkname raises exception""" 75​ """Checks if missing checkname raises exception"""
68 with pytest.raises(TaskotronDirectiveError): 76​ with pytest.raises(TaskotronDirectiveError):
69 envdata = deepcopy(self.ref_envdata) 77​ envdata = deepcopy(self.ref_envdata)
70 del(envdata['checkname']) 78​ del(envdata['checkname'])
71 79​
72 self.test_rdb.process(self.ref_input, envdata) 80​ self.test_rdb.process(self.ref_input, envdata)
73 81​
74 def test_failed_tap_import(self, monkeypatch): 82​ def test_failed_tap_import(self, monkeypatch):
75 """Checks if failed TAP import raises exception""" 83​ """Checks if failed TAP import raises exception"""
76 def mock_raise(self, *args, **kwargs): 84​ monkeypatch.setattr(check, 'import_TAP',
77 raise TaskotronValueError("Testing Error") 85​ exception_raiser(TaskotronValueError("Testing Error")))
78 ​
79 monkeypatch.setattr(check, 'import_TAP', mock_raise) ​
80 86​
81 with pytest.raises(TaskotronDirectiveError): 87​ with pytest.raises(TaskotronDirectiveError):
82 self.test_rdb.process(self.ref_input, self.ref_envdata) 88​ self.test_rdb.process(self.ref_input, self.ref_envdata)
83 89​
84 def test_report(self): 90​ def test_report(self):
85 """Checks whether TAP is correctly mapped to the reporting method's 91​ """Checks whether TAP is correctly mapped to the reporting method's
86 arguments.""" 92​ arguments."""
87 93​
88 self.test_rdb.process(self.ref_input, self.ref_envdata) 94​ self.test_rdb.process(self.ref_input, self.ref_envdata)
89 95​
90 # Given the input data, the resultsdb should be called once, and only 96​ # Given the input data, the resultsdb should be called once, and only
91 # once, calling "create_result". 97​ # once, calling "create_result".
92 # This assert failing either means that more calls were added in the 98​ # This assert failing either means that more calls were added in the
93 # source code, or that a bug is present, and "create_result" is 99​ # source code, or that a bug is present, and "create_result" is
94 # called multiple times. 100​ # called multiple times.
101​ # we expect rdb to be called 4 times: create job, check for testcase,
102​ # report result and complete job
95 103​
96 assert len(self.stub_rdb.calls()) == 1 104​ assert len(self.stub_rdb.calls()) == 4
97 105​
98 # Select the first call of "create_result" method. 106​ # Select the first call of "create_result" method.
99 # This could be written as self.stub_rdb.calls()[0] at the moment, but 107​ # This could be written as self.stub_rdb.calls()[0] at the moment, but
100 # this is more future-proof, and accidental addition of resultsdb 108​ # this is more future-proof, and accidental addition of resultsdb
101 # calls is handled by the previous assert. 109​ # calls is handled by the previous assert.
102 call = [call for call in self.stub_rdb.calls() if call[0] == 'create_result'][0] 110​ call = [call for call in self.stub_rdb.calls() if call[0] == 'create_result'][0]
103 # Select the keyword arguments of that call 111​ # Select the keyword arguments of that call
104 call_data = call[2] 112​ call_data = call[2]
105 113​
106 assert call_data['job_id'] == self.ref_envdata['resultsdb_job_id'] 114​ assert call_data['job_id'] == self.ref_jobid
107 assert call_data['testcase_name'] == self.ref_envdata['checkname'] 115​ assert call_data['testcase_name'] == self.ref_envdata['checkname']
108 assert call_data['outcome'] == self.cd.outcome 116​ assert call_data['outcome'] == self.cd.outcome
109 assert call_data['summary'] == self.cd.summary 117​ assert call_data['summary'] == self.cd.summary
110 assert call_data['log_url'] == "http://example.com" #FIXME 118​ assert call_data['log_url'] == "http://example.com" #FIXME
111 assert call_data['item'] == self.cd.item 119​ assert call_data['item'] == self.cd.item
112 assert call_data['type'] == self.cd.report_type 120​ assert call_data['type'] == self.cd.report_type
113 121​
114 assert 'output' not in call_data.keys() 122​ assert 'output' not in call_data.keys()
115 123​
116 for key in self.cd.keyvals.keys(): 124​ for key in self.cd.keyvals.keys():
117 assert call_data[key] == self.cd.keyvals[key] 125​ assert call_data[key] == self.cd.keyvals[key]
118 126​
127​ def test_create_job(self):
128​ # make sure that the proper API calls are made to create a resultsdb job
129​ test_jobdata = self.test_rdb.create_resultsdb_job(self.ref_envdata['checkname'],
130​ self.ref_refurl)
131​
132​ assert test_jobdata == self.ref_jobdata
133​
134​ rdb_calls = self.stub_rdb.calls()
135​ # we expect only one call to resultsdb when creating a job
136​ assert len(rdb_calls) == 1
137​ assert rdb_calls[0][0] == 'create_job'
138​
139​ def test_complete_job(self):
140​ # make sure that the proper API calls are made to complete the job
141​ self.test_rdb.complete_resultsdb_job(self.ref_jobid)
142​
143​ rdb_calls = self.stub_rdb.calls()
144​ # we expect only one call to resultsdb when completing a job
145​ assert len(rdb_calls) == 1
146​ assert rdb_calls[0][0] == 'update_job'
147​ assert rdb_calls[0][2]['status'] == 'COMPLETED'
148​
149​ def test_ensure_testcase_creation_notexist(self):
150​ # check to see if the testcase is created in the case that it doesn't
151​ # already exist
152​
153​ self.stub_rdb.get_testcase =exception_raiser(ResultsDBapiException('Testcase not found'))
154​
155​ self.test_rdb.ensure_testcase_exists(self.ref_envdata['checkname'])
156​
157​ rdb_calls = self.stub_rdb.calls()
158​
159​ # dingus doesn't record calls to exception_raiser
160​ assert len(rdb_calls) == 1
161​ assert rdb_calls[0][0] == 'create_testcase'
162​
163​ def test_ensure_testcase_creation_exists(self):
164​ # check to make sure that a testcase is _not_ created in the case that
165​ # it already exists
166​
167​ self.test_rdb.ensure_testcase_exists(self.ref_envdata['checkname'])
168​
169​ rdb_calls = self.stub_rdb.calls()
119 170​
171​ # dingus doesn't record calls to exception_raiser
172​ assert len(rdb_calls) == 1
173​ assert rdb_calls[0][0] == 'get_testcase'