PR#93: Add 'scenario' to identifiers in get_prev_result - taskotron/resultsdb

taskotron / resultsdb

#93 Add 'scenario' to identifiers in get_prev_result

Merged 6 years ago by jskladan. Opened 6 years ago by adamwill.

taskotron/ adamwill/resultsdb prev-scenario into develop

Add 'scenario' to identifiers in get_prev_result

Adam Williamson • 6 years ago

2067ce5

resultsdb/controllers/api_v2.py

file modified

+3 -3

		`@@ -678,14 +678,14 @@`

		`def get_prev_result(result):`
		`"""`
		`- Find previous result with the same testcase, item, type, and arch.`
		`- Return None if no result is found.`
		`+ Find previous result with the same testcase, item, type, arch and`
		`+ scenario. Return None if no result is found.`
		`"""`
		`q = db.session.query(Result).filter(Result.id != result.id)`
		`q = q.filter_by(testcase_name=result.testcase_name)`

		`for result_data in result.data:`
		`- if result_data.key in ['item', 'type', 'arch']:`
		`+ if result_data.key in ['item', 'type', 'arch', 'scenario']:`
		`alias = db.aliased(ResultData)`
		`q = q.join(alias).filter(`
		`db.and_(alias.key == result_data.key, alias.value == result_data.value))`

testing/functest_create_fedmsg.py

file modified

+16

		`@@ -88,10 +88,12 @@`
		`self.ref_result_item = 'perl-Specio-0.25-1.fc26'`
		`self.ref_result_type = 'koji_build'`
		`self.ref_result_arch = 'x86_64'`
		`+ self.ref_result_scenario = 'x86_64.efi'`
		`self.ref_result_data = {`
		`'item': self.ref_result_item,`
		`'type': self.ref_result_type,`
		`'arch': self.ref_result_arch,`
		`+ 'scenario': self.ref_result_scenario,`
		`'moo': ['boo', 'woof'],`
		`}`
		`self.ref_result_ref_url = 'http://example.com/testing.result'`
		`@@ -144,6 +146,8 @@`
		`assert result_data.value == self.ref_result_type`
		`if result_data.key == 'arch':`
		`assert result_data.value == self.ref_result_arch`
		`+ if result_data.key == 'scenario':`
		`+ assert result_data.value == self.ref_result_scenario`

		`self.helper_create_result()`
		`prev_result = apiv2.get_prev_result(self.ref_result_obj)`
		`@@ -158,6 +162,8 @@`
		`assert result_data.value == self.ref_result_type`
		`if result_data.key == 'arch':`
		`assert result_data.value == self.ref_result_arch`
		`+ if result_data.key == 'scenario':`
		`+ assert result_data.value == self.ref_result_scenario`

		`ref_outcome = 'FAILED'`
		`if self.ref_result_outcome == ref_outcome:`
		`@@ -175,6 +181,8 @@`
		`assert result_data.value == self.ref_result_type`
		`if result_data.key == 'arch':`
		`assert result_data.value == self.ref_result_arch`
		`+ if result_data.key == 'scenario':`
		`+ assert result_data.value == self.ref_result_scenario`

		`def test_get_prev_result_different_item(self):`
		`data = copy.deepcopy(self.ref_result_data)`
		`@@ -200,6 +208,14 @@`
		`prev_result = apiv2.get_prev_result(self.ref_result_obj)`
		`assert prev_result is None`

		`+ def test_get_prev_result_different_scenario(self):`
		`+ data = copy.deepcopy(self.ref_result_data)`
		`+ data['scenario'] = data['scenario'] + '.fake'`
		`+ self.helper_create_result(data=data)`
		`+`
		`+ prev_result = apiv2.get_prev_result(self.ref_result_obj)`
		`+ assert prev_result is None`
		`+`
		`def test_get_prev_result_different_testcase_name(self):`
		`self.helper_create_result(testcase={'name': self.ref_testcase_name + '.fake'})`

adamwill commented 6 years ago

As @ralph pointed out, the get_prev_result logic should also
use 'scenario' as one of the identifiers when deciding if a
result is 'the same as' a previous one; this is explicitly what
the key is intended for, and it is necessary for openQA tests,
as they may have the same testcase name, arch, item and type,
but be for a different firmware type, for instance. This may be
preventing us from emitting fedmsgs for some openQA results.

Signed-off-by: Adam Williamson awilliam@redhat.com

jskladan commented 6 years ago

While I understand, this is (probably) a temporary solution "for the time before we get this right", it seems to be getting in the "bloated" direction a bit, so I'd like to see some "FIXME"s there to point out that this will need to be solved properly. The original message emitting code is very Taskotron-y, and adding OpenQA-y layer just on top of that works (for now), but we IMO start being even more "arbitrary".

Nevertheless, the "proper" solution will most certainly be far more complex than this (which solves the current issue), so I'm happy with this being (properly marked) temporary solution (and we know how temp. solutions tend to stick around...).

adamwill commented 6 years ago

I mean, I didn't exactly write it to 'solve the issue', it was just something @ralph pointed out that seems to be a clear fix to the current implementation as it exists. If anything the FIXME should go around the whole...mechanism, I guess. This was really just an 'obvious correction' commit - as long as this thing is around, it should probably do this.

remember scenario isn't really just openqa-y, it was discussed as a kinda generic requirement, and I think we were actually going to use it for taskotron results too (we just sorta forgot).

jskladan commented 6 years ago

Yeah, that's what I meant - that the whole "this is how we do messaging" should be revised. Not the one change with scenario.
I do not really remember scenario being discussed as generic (but I'm like Jon Snow, I know nothing...), and can't really think of way of using it for the taskotron process, but that is pretty out of the scope of the change :)
Anyway, I'll merge the commit and add the "hey, the messaging need to be figured out properly" comments myself to the whole thing :)
THX for the patch, @adamwill !