Differential D1218

add SKIPPED outcome
AbandonedPublic
Actions

Authored by kparal on Mon, Jul 10, 6:09 AM.

Details

Reviewers
jskladan
dcallagh
Summary

RPMDiff is a comparison tool which can analyse differences between two packages to find problems. We are currently importing the results from RPMDiff comparison into ResultsDB. However in some cases, the RPMDiff tests have to be skipped because there is no baseline package to compare against. In that case, our tools consuming the results need to be able to distinguish between two possibilities: the result is missing because it hasn't finished running yet, or the result will *never* appear because the tool has decided it cannot run the test. Thus SKIPPED.

Test Plan
  1. create an empty Postgres database
  2. initialise it with init_db using current develop branch (Alembic revision 4dbe714897fe)
  3. run upgrade_db with this patch, use \dT+ resultoutcome to verify enum type now permits SKIPPED
  4. insert a result with the new SKIPPED outcome
  5. run alembic downgrade 4dbe714897fe, use \dT+ resultoutcome to verify SKIPPED is no longer a value in the enum, also verify that the result in step 4 is mapped to INFO outcome

Diff Detail

Repository
rRSDB resultsdb
Lint
Lint Skipped
Unit
Unit Tests Skipped
dcallagh created this revision.Mon, Jul 10, 6:09 AM
kparal added a subscriber: kparal.Mon, Jul 10, 11:47 AM

Out of curiosity, why not use PASSED for them? I'm not saying SKIPPED is a bad idea (I encountered this myself lately and would use it if it was present), but in my case the two outcomes had the same meaning in the end (in terms of gating, both skipped and passed means thumbs up). What is your scenario where you need to distinguish the two?

kparal added a comment.Mon, Jul 10, 11:51 AM

Also, any reason for skipping the test suite and lint when creating this diff? I don't see any unit test failures, but some lint warnings are present.

dcallagh added a comment.Mon, Jul 10, 9:44 PM

Ohh no, skipping the test suite/linter was accidental... due to me not understand Phabricator UI I guess. :-) I didn't see any button to skip them or run them so not sure what I did wrong.

dcallagh added a comment.Mon, Jul 10, 9:46 PM

PASSED would work I guess. The distinction is really just for the humans who might be auditing the results. It would be surprising if the tooling claims that the RPMDiff comparison passed, but when you click through to look at it you see it actually didn't test anything at all.

kparal added a comment.Tue, Jul 11, 5:50 AM
In D1218#22627, @dcallagh wrote:

Ohh no, skipping the test suite/linter was accidental... due to me not understand Phabricator UI I guess. :-) I didn't see any button to skip them or run them so not sure what I did wrong.

If you used arc diff develop when creating this diff, it should be run automatically. If you created this diff manually through the web ui, it's not run. Either way, never mind, just a mental note to check this before committing.

In D1218#22628, @dcallagh wrote:

PASSED would work I guess. The distinction is really just for the humans who might be auditing the results. It would be surprising if the tooling claims that the RPMDiff comparison passed, but when you click through to look at it you see it actually didn't test anything at all.

I see. I'll let @jskladan decide here. I don't object to adding SKIPPED outcome. I believe more people will be asking for it in the future as well.

dcallagh added a comment.Tue, Jul 11, 6:45 AM

Ah sorry, I was too lazy to set up the arc diff stuff so I just pasted the diff in by hand. I guess that is why.

I can try and run the linting steps by hand and fix up whatever the issue is...

jskladan added a comment.EditedTue, Jul 11, 11:26 AM

@dcallagh Well, I'm not fundamentally absolutely against it, but I'd be happier with you using INFO (which is already present) with "test was skipped" in note - especially if you say that it's supposed to be a pass anyway.
The intended semantics of the result values is
PASSED - everything ok, no questions asked
INFO - the same as PASSED for automation purposes, but a yellow flag for human consumers
NEEDS_INSPECTION - treated as FAILED in automation, red flag for human consumers
FAILED - something really went sideways

As described in the docs.
Given the intent, I believe this is the right, and elegant solution.

The result values are also semantically ordered. Meaning that when you aggregate results, the "worse" value overrides the "better" value (aka if there are PASSED, INFO and FAILED results, the aggregate would be FAILED). Where do you think SKIPPED goes in this use case?

Once again - I could be persuaded to include SKIPPED but from my PoW it is somewhere between redundant and pointless.

dcallagh added a comment.Tue, Jul 11, 9:55 PM

INFO with a note makes sense to me -- assuming that we actually display that note to humans when we are presenting the results to them.

For example in Bodhi right now I think the note does *not* appear anywhere... so that might be something we should add?

kparal added a comment.EditedWed, Jul 12, 10:43 AM
In D1218#22650, @dcallagh wrote:

For example in Bodhi right now I think the note does *not* appear anywhere... so that might be something we should add?

Sure it does. Look at e.g. https://bodhi.fedoraproject.org/updates/FEDORA-2017-364f4be949 .

dist.rpmgrill.build-log   418 issues found	
dist.rpmgrill.man-pages  20 issues found

Those are notes already being used for rpmgrill.

Please note that note is considered to be a mainly free-form text useful for human consumption. If you want to process this automatically, it might be much better if you add e.g. skipped=true extra arg to the result and then the tool can have a look whether such extra arg exists (if needed). Or you can combine both together, of course.

I'd personally keep the main outcome as PASSED, add just add the "skipped" note, because INFO is supposed to draw more attention (next version of Bodhi will display those as blue 'i' symbols), and that's something you probably don't want to do for skipped tests (boring!). But that's just my idea how things can be done.

kparal added a comment.Wed, Aug 2, 2:09 PM

@dcallagh , do you still need SKIPPED outcome or is the proposed solution good enough for you?

dcallagh added a comment.Wed, Aug 2, 11:54 PM

The reasons against SKIPPED make sense to me. I haven't heard any feedback (either negative or positive) from the other teams who are producing the results which will be skipped. So let's just assume that they are fine with it too. We can drop this.

dcallagh added a comment.Wed, Aug 2, 11:54 PM

I can't figure out which button to press to reject a change in Phab so I'll let you do that. :-)

kparal commandeered this revision.Thu, Aug 3, 7:31 AM
kparal added a reviewer: dcallagh.

There should be "abandon revision" action in the Action drop down menu. I'll do it.

kparal abandoned this revision.Thu, Aug 3, 7:31 AM

Revision Contents

Diff 3090

View Options

APIDOCS.apiary

Context not available.
28 * `ref_url` instead of `log_url` 28​ * `ref_url` instead of `log_url`
29 * `result_data` replaced with `data` 29​ * `result_data` replaced with `data`
30 * `summary` replaced with `note` 30​ * `summary` replaced with `note`
31 * only `PASSED, FAILED, INFO, NEEDS_INSPECTION` are now valid `outcome` values 31​ * only `PASSED`, `FAILED`, `INFO`, `NEEDS_INSPECTION`, and `SKIPPED` are now valid `outcome` values
32 * `groups` is a list of `Group` uuids which the `Result` is part of 32​ * `groups` is a list of `Group` uuids which the `Result` is part of
33 * When submitting new `Result`, `Testcase` and `Group` \[`resource`\] can be represented either by the unique identifier \[`uid`\] 33​ * When submitting new `Result`, `Testcase` and `Group` \[`resource`\] can be represented either by the unique identifier \[`uid`\]
34 (`Testcase.name`, `Group.uuid`), or by `object` containing the `uid`, and any other applicable `resource`'s attributes 34​ (`Testcase.name`, `Group.uuid`), or by `object` containing the `uid`, and any other applicable `resource`'s attributes
Context not available.
130 + FAILED 130​ + FAILED
131 + INFO 131​ + INFO
132 + NEEDS_INSPECTION 132​ + NEEDS_INSPECTION
133​ + SKIPPED
133 + testcases: dist.rpmlint (string, optional) 134​ + testcases: dist.rpmlint (string, optional)
134 - Multiple values can be provided, separate by coma to get `or` filter based on all the values provided: `...&testcases=dist.rpmlint,dist.depcheck` 135​ - Multiple values can be provided, separate by coma to get `or` filter based on all the values provided: `...&testcases=dist.rpmlint,dist.depcheck`
135 - `like` filter with `*` as wildcard: `...&testcases:like=dist.*` 136​ - `like` filter with `*` as wildcard: `...&testcases:like=dist.*`
Context not available.
537- FAILED 538​- FAILED
538- INFO - represents a "soft pass" - use to treat as `"PASSED"` by automation/gating, and flag for human review 539​- INFO - represents a "soft pass" - use to treat as `"PASSED"` by automation/gating, and flag for human review
539- NEEDS_INSPECTION - use to treat as `"FAILED"` by automation/gating, and flag for human review 540​- NEEDS_INSPECTION - use to treat as `"FAILED"` by automation/gating, and flag for human review
541​- SKIPPED
540 542​
541## Result GET (object) 543​## Result GET (object)
542- id: 7484989 (number, required) - Unique identifier. Assigned by API at the moment of creation. 544​- id: 7484989 (number, required) - Unique identifier. Assigned by API at the moment of creation.
Context not available.
View Options

alembic/versions/262e09cc6a5f_add_skipped_outcome.py

  • This file was added.
1​"""add SKIPPED outcome
2​
3​Revision ID: 262e09cc6a5f
4​Revises: 4dbe714897fe
5​Create Date: 2017-07-10 14:17:07.011152
6​
7​"""
8​
9​# revision identifiers, used by Alembic.
10​revision = '262e09cc6a5f'
11​down_revision = '4dbe714897fe'
12​branch_labels = None
13​depends_on = None
14​
15​from alembic import op
16​import sqlalchemy as sa
17​
18​old_values = ('PASSED', 'INFO', 'FAILED', 'NEEDS_INSPECTION')
19​new_values = ('PASSED', 'INFO', 'FAILED', 'NEEDS_INSPECTION', 'SKIPPED')
20​
21​old_enum = sa.Enum(*old_values, name='resultoutcome')
22​tmp_enum = sa.Enum(*new_values, name='resultoutcome_tmp')
23​new_enum = sa.Enum(*new_values, name='resultoutcome')
24​
25​
26​def upgrade():
27​ # this migration is postgresql specific and fails on sqlite
28​ if op.get_bind().engine.url.drivername.startswith("postgresql"):
29​ tmp_enum.create(op.get_bind(), checkfirst=False)
30​ op.execute('ALTER TABLE result ALTER COLUMN outcome TYPE resultoutcome_tmp '
31​ ' USING outcome::text::resultoutcome_tmp')
32​ old_enum.drop(op.get_bind(), checkfirst=False)
33​ new_enum.create(op.get_bind(), checkfirst=False)
34​ op.execute('ALTER TABLE result ALTER COLUMN outcome TYPE resultoutcome '
35​ ' USING outcome::text::resultoutcome')
36​ tmp_enum.drop(op.get_bind(), checkfirst=False)
37​
38​
39​def downgrade():
40​ # this migration is postgresql specific and fails on sqlite
41​ if op.get_bind().engine.url.drivername.startswith("postgresql"):
42​ op.execute("UPDATE result SET outcome='INFO' WHERE outcome='SKIPPED'")
43​
44​ tmp_enum.create(op.get_bind(), checkfirst=False)
45​ op.execute('ALTER TABLE result ALTER COLUMN outcome TYPE resultoutcome_tmp '
46​ ' USING outcome::text::resultoutcome_tmp')
47​ new_enum.drop(op.get_bind(), checkfirst=False)
48​ old_enum.create(op.get_bind(), checkfirst=False)
49​ op.execute('ALTER TABLE result ALTER COLUMN outcome TYPE resultoutcome '
50​ ' USING outcome::text::resultoutcome')
51​ tmp_enum.drop(op.get_bind(), checkfirst=False)
View Options

resultsdb/models/results.py

Context not available.
27__all__ = ['Testcase', 'Group', 'Result', 'ResultData', 'GroupsToResults', 'RESULT_OUTCOME'] 27​__all__ = ['Testcase', 'Group', 'Result', 'ResultData', 'GroupsToResults', 'RESULT_OUTCOME']
28 28​
29 29​
30RESULT_OUTCOME = ('PASSED', 'INFO', 'FAILED', 'NEEDS_INSPECTION') 30​RESULT_OUTCOME = ('PASSED', 'INFO', 'FAILED', 'NEEDS_INSPECTION', 'SKIPPED')
31JOB_STATUS = [] 31​JOB_STATUS = []
32 32​
33 33​
Context not available.
View Options

testing/functest_api_v20.py

Context not available.
429 assert r.status_code == 201 429​ assert r.status_code == 201
430 assert data == self.ref_result 430​ assert data == self.ref_result
431 431​
432​ def test_create_result_skipped(self):
433​ self.test_create_group()
434​ self.test_create_testcase()
435​
436​ r, data = self.helper_create_result(outcome='SKIPPED')
437​ assert r.status_code == 201
438​ assert data['outcome'] == 'SKIPPED'
439​
432 def test_create_result_missing_data(self): 440​ def test_create_result_missing_data(self):
433 ref_data = json.dumps({'outcome': 'FAKEOUTCOME'}) 441​ ref_data = json.dumps({'outcome': 'FAKEOUTCOME'})
434 r = self.app.post('/api/v2.0/results', data=ref_data, content_type='application/json')442​ r = self.app.post('/api/v2.0/results', data=ref_data, content_type='application/json')
Context not available.