Aren't we complicating things needlessly? I think that reporting rpmlint results //per RPM// is too detailed. Why can't we report rpmlint results per build? The maintainer looks after all the RPM files that are created from this package. So you tell him FAILED and provide the whole build log to him. If we format the output well, it should be easy to see which RPM file caused the failure (the default rpmlint output is fine, in this case, no changes needed).

Is this all about maintainer convenience (he doesn't need to see any other output except for the failed RPM), or do we have other use cases for rpmlint results stored per RPM? If only the first is true, I think we should not get too detailed and complicate the reporting structure.

If we end up with overall results for some reason, I think we should create them automatically. We know the build ENVR from the input args, and the result should be the highest priority result from the list of computed results.
PASSED < INFO < FAILED < NEEDS_INSPECTION < ABORTED < CRASHED

Yeah, the TAP format needs some more definition. There are a couple of things about your proposal that I'm not sure about:
* why is the 'update_title' needed? It seems pretty specific to bodhi-related reporting. Can't resultsdb reporting be done in a non-bodhi-specific way?
* I don't like having the 'do_report' in the task output. The logic to avoid duplicates should be in resultsdb, not the task/runner.

Tim,

ad update_title - do you see any other clear way to specify (especially for multi-result tests like depcheck), which "reportable result" belongs to which update? For single-envr/single-update tests it can be easily done by (say) deciding, that the envr/update must be set by a specific parameter when running the task, so we can extract it automagically. But tests which can report results for multiple ENVRs/updates (like depcheck) should specity that "this test result is tied to this update/build/iso/whatever".

In other words - how do you intend on getting the "identifier" of the tested part (be it package, rpm file, build, update, iso) to the results, other then specifying it in the TAP output? Since (at least IMHO) if the TAP output does not contain any identifier of "what was actually tested here", then it is (IMHO) near to useless out of the runinng task's context.

I do not see any connection with resultsdb though. This ticket is an outcome of me working on the bodhi reporting, so it kind of makes sense to have the bodhi reporting behave a bit in a bodhi-specific way.

ad do report - I do strongly disagree with "The logic to avoid duplicates should be in resultsdb". At least without any specification of "what is, generally and in all cases, a duplicate". ResultsDB should not be (and will not be) aware of the results semantics. Taking Rpmlint as an example - if a developer decides to put granular results in the TAP output (i.e. result for each rpm file), then there is absolutely no problem (at resultsdb's side) to store all the results. I just tend to believe, that the developer should also provide an overall result of the runned task (i.e. the overall result for the Rpmlint run), and that the overall result should be easily distinguishable from the 'subresults'. To represent this "meaningfull" result, I chose the do_report (I do feel attached to the name, though), to represent this fact.

ad update_title - do you see any other clear way to specify (especially for multi-result tests like depcheck), which "reportable result" belongs to which update? For single-envr/single-update tests it can be easily done by (say) deciding, that the envr/update must be set by a specific parameter when running the task, so we can extract it automagically. But tests which can report results for multiple ENVRs/updates (like depcheck) should specity that "this test result is tied to this update/build/iso/whatever".

I'd rather do it through TAP structure. As an example, envision an update foo-1.2-3.fc20 as tested on x86_64:

not ok - foo-1.2-3.fc20 x86_64 # FAIL depcheck for update foo-1.2-3.fc20
---
foo-base-1.2-3.fc20.x86_64:
ERRORS:
- some depcheck error
- some other error
WARN:
- some depcheck warn
python-foo-1.2-3.fc20.noarch:
- OK
...

That's one TAP message containing the status for the update - each RPM's status is listed in the YAML. Unless I'm forgetting something, everything needed for a report is contained in the message.

In other words - how do you intend on getting the "identifier" of the tested part (be it package, rpm file, build, update, iso) to the results, other then specifying it in the TAP output? Since (at least IMHO) if the TAP output does not contain any identifier of "what was actually tested here", then it is (IMHO) near to useless out of the runinng task's context.

In that case, the update title is in the result, but we could move that around as long as it's in a consistent place for parsing.

I think that this would be a better way to report. It fits how I understand resultsdb to work and the bodhi reporting can be written to parse the messages consistently without needing extra summary messages to communicate run status.

I do not see any connection with resultsdb though. This ticket is an outcome of me working on the bodhi reporting, so it kind of makes sense to have the bodhi reporting behave a bit in a bodhi-specific way.

Reporting, sure. I just don't want to have drastically different formats for bodhi-based tasks when compared to tasks which aren't reporting to bodhi.

ad do report - I do strongly disagree with "The logic to avoid duplicates should be in resultsdb". At least without any specification of "what is, generally and in all cases, a duplicate". ResultsDB should not be (and will not be) aware of the results semantics.

Yeah, I had things mixed up in my head and was thinking that bodhi reporting was going to be done from resultsdb. My bad.

Taking Rpmlint as an example - if a developer decides to put granular results in the TAP output (i.e. result for each rpm file), then there is absolutely no problem (at resultsdb's side) to store all the results. I just tend to believe, that the developer should also provide an overall result of the runned task (i.e. the overall result for the Rpmlint run), and that the overall result should be easily distinguishable from the 'subresults'. To represent this "meaningfull" result, I chose the do_report (I do feel attached to the name, though), to represent this fact.

However, I disagree with you here. I think that TAP messages should be the only unit of output that is used for reporting. If something shouldn't be reported, it shouldn't be specified as part of the task. I can't think of a use case where we'd really want to have conditional reporting to bodhi or resultsdb.

Tim,

I really like the idea of having the results in TAP structure!

Maybe I should have been a bit clearer (now that I re-read what I wrote in the beginning) - I do not have any problem with the reporting being as auto-magical as it can be. I'll try to sum up what pieces of information we (IMHO) need, and we can then see where it's stored and what (if any) pieces of the information need to be in the TAP output, and which can just be taken from the runtime environment.

If I make my mind around testing updates (which is what we mostly aim to do now, IMHO), I see these, as the reasonable minimal set of information

# Test name - I firmly believe that this can be (and should be) taken from the runtime data. All the reporting directives will have access to it, so no problem here. Also, having the test name in the comments section of the TAP result (exactly as you drafted) is a good idea for later usage.
# Result - I think that we should be able to provide not only the "crude" TAP ok/not ok. What would you think about adding (non-mandatory) keyval "result" to the YAML extradata, where you could specify the "extended" result (WARN, INFO, ...). My idea is, that if the "extended result" is not provided, ok/not ok is simply mapped onto PASS/FAIL. Once the extended result is provided, the ok/not ok is simply ignored, and the "result" keyval is used as the respective result. To make this a bit more reader-friendly, we could either divide our extended results into ok/not ok categories, and make the helper class (CheckDetail) put the respective ok/not ok based on the result, or just put not ok to anything else than plain PASS. I do not see this as that big of a deal, though.
# What was tested - using the Description part of TAP result seems absolutely reasonable. Bodhi reporting can "just assume" that this is an update title and arch (as you have in your scheme), and it does not really bind us by using specific keyword. So +1 here

I'd strongly support having a "TAP result" for each "reportable result", what is (IIUC) exactly what you proposed. This then means, that there is no practical difference between single and multiple update tests (in the TAP output context), as we just go through the TAP results, and each one is treated as a standalone result (or "unit of result" as funny as it sounds :D).

From the ResultsDB's point of view, we need some more pieces of extra-data, and it is the Job ID, under which the results should be filed, and the URL pointing to full logs. But this is most definitely something, that needs to be taken from the runtime environment.

Talking about ResultsDB - I'm stil unsure how to treat the TAP output with ResultsDB in mind, having the example you provided (copying just for reference):

not ok - foo-1.2-3.fc20 x86_64  # FAIL depcheck for update foo-1.2-3.fc20
  ---
  foo-base-1.2-3.fc20.x86_64:
    ERRORS:
    - some depcheck error
    - some other error
    WARN:
    - some depcheck warn
  python-foo-1.2-3.fc20.noarch
  ...

I would like to keep the reporting as test-unaware as possible, while also storing enough information, so the results are actually of some use :) My main headaches now are:
# Testable-specific "extra data" - for example arch for update/build
# Deciding what information from the TAP-YAML to store in ResultsDB's extradata

The is closely related to the what was tested generalist point. As much as I do not see a problem of the "testable" being described/identified by the TAP description part, I'm not sure how to add the (for example) arch (and I know, that arch is/can be provided in the runners runtime data), in a way that is not requiring the parser to be aware of the "semantics" of each of the Checks TAP outputs. My initial idea (that I'm not that fond of) is just treating all the TAP-YAML data as key-value pairs which will then be stored in ResulsDB as such, meaning that in the aforementioned TAP output, ResultsDB result would contain something like:

{
    "outcome": 'PASSED',
    "testcase_name": 'depcheck,
    "summary": 'FAIL depcheck for update foo-1.2-3.fc20'
    "result_data": {
                     'tested': 'foo-1.2-3.fc20',
                     'arch': 'x86_84',
                     'foo-base-1.2-3.fc20.x86_64': 'ERRORS: ...',
                     'python-foo-1.2-3.fc20.noarch': 'OK'
                     },
    "job_id": int,
    "log_url": string
}

The good thing about is that, the reporting directive must not be at all aware of any semantics of the TAP results, the bad thing is that ResultsDB would be storing (possibly) unnecessary amounts of data.

Do you have any ideas, tips? To be honest, I feel a bit lost here.

Also, re-reading what I initially wrote - it seems that my reply had quite a harsh tone, which I was not my intent, but I still feel like I should apologize for being a bit more grumpy, than I should have been.

I agree with what Josef wrote about where to get data from. The only thing I'm wondering is if we want to modify the TAP generator to more closely align with the result states that we're using in CheckDetail.

One of the other things I'm starting to wonder is whether we want to bother with TAP at all. Sure, it was designed for reporting test results but we're already messing with it by using the embedded yaml, so why not just go the whole way and use more raw json or yaml so we can get rid of the restrictions imposed on us by using TAP. It's not like we're using some out-of-the-box TAP parser anyways and I don't think that writing up a yaml/json wrapper would be all that much more work than making TAP do what we want it to do. Is there some major benefit to using TAP that I'm missing here other than not having to design our own format?

As far as what to put into resultsdb, one technical nit - won't using envr for keys cause problems with the key-value index in resultsdb since the length is set for 20 chars and 50 chars, respectively. Do we really want to be indexing all that data in a generic form?

That being said, I'm not sure I have any better ideas given what we currently have. I suspect that some rework of resultsdb is going to be in order at some point but IIRC, that was something we discussed at Flock last year.

For going forward, I'm OK with going forward with what you've proposed here. We'll have to see how well it works in staging.

it seems that my reply had quite a harsh tone, which I was not my intent, but I still feel like I should apologize for being a bit more grumpy, than I should have been.

No worries, it happens. It didn't seem overly grumpy to me :)

Ad whether to use TAP at all - IMHO we wanted to go for some standardized format. Although using embedded YAML is not IMHO really messing with the format (it's a part of the TAP13 protocol http://podwiki.hexten.net/TAP/TAP13.html?page=TAP13), I see where you are going with this. And to be honest, I do agree. Since most of the data we are (at least as far as I can tell) going to use will be in the embedded YAML, we can probably equally easily just create the same structure in pure YAML. And it does not necessarily need to look that different from what TAP looks like (I'm not saying that it needs to look like it, though).

But I am not overly attached to any of these (TAP or pure YAML) possibilities. Having pure YAML removes some parsing overhead, using TAP gives us standardized format. For me, pure YAML would have a slight edge (i.e. getting rid of 'custom' libraries), but I'd like to have comments from the rest of you guys.

Ad resultsdb - I'm still sorting that out. My problem here is not that much database related (or related to how the storage is implemented, I'll be using ResultsDB as an example, but this is imho quite results store agnostic), but how to decide, which parts of the YAML data are to be stored. My first idea (which I discarded rather soon, though) was that we might define a set of keys, that would get stored automatically, and the rest of it would be ignored (e.g. it would only be in the TAP/YAML output, but not stored in ResultsDB).

As much as the actual idea is flawed (IMHO), it corresponds with my idea of the ResultsDBs role - it should be storing the information, that enables us to query and aggregate the results of tests for specific 'testable'. E.G. for the update gating, we need stuff like ENVR and arch. On top of that (if the information is present at the time of the check-run) we might want to have the update-id stored, ... When gating updates, we can then have an intermediatory layer, that (when given update id/title) will know what our tests do, and what kind of information is stored in ResultsDB (let's say that Upgradepath results are stored per-build, Depcheck is stored per-update). That middleware (?) would then query Bodhi, to get the list of builds for the specific update, query ResultsDB for the Depcheck's result of the update, the results of Upgradepath for each build, and then will aggregate the resuls (e.g. all builds need to be tested, and all tests need to be passed), and based on the data, it would say "gate/don't gate".

What this means (at least in my head) is, that we don't really need to store all the specific errors which caused the one depcheck run to fail. We need to store that the depcheck run failed on the specified testable (e.g. update and architecture). Everything else is just noise.

What I'd like to solve (and I'm not sure how to do it "pretty") is giving the check-developer a way to specify "These parts of the output (or these key-value pairs), I want to have stored for later querying." Specifying some standard set of keys would be great. E.g. outcome for "how it ended", tested storing the identifier of what was tested (envr, iso name, repo url, whatever we decide to test), arch as a standard key for storing architecture, and quite probably a few more, that I can't think of right now.

The other variant (that may be a bit better) is treating it the completely opposite way (while still having some "standardized names for keys" as a guideline, of course), where we could say "whatever you put into key named XYZ (e.g. logs) we won't be storing in our datastore", that would be (IMHO) quite simple for the check-developers, as the only thing they'd need to care about is putting the "non-reportable" data in the right place. E.G.

not ok - foo-1.2-3.fc20 x86_64  # FAIL depcheck for update foo-1.2-3.fc20
  ---
  outcome: NEEDS_INSPECTION
  tested: foo-1.2-3.fc20
  arch: x86_64
  whatever: "Yaay, I'm awesome"
  logs:
    foo-base-1.2-3.fc20.x86_64:
      ERRORS:
      - some depcheck error
      - some other error
      WARN:
      - some depcheck warn
    python-foo-1.2-3.fc20.noarch
    ...

Which would then store something like this in the resultsdb store:
```{
"outcome": 'NEEDS_INSPECTION',
"testcase_name": 'depcheck,
"summary": 'FAIL depcheck for update foo-1.2-3.fc20'
"result_data": {
'tested': 'foo-1.2-3.fc20',
'arch': 'x86_84',
'whatever': "Yaay, I'm awesome",
},
"job_id": int,
"log_url": string
}

```

Ad whether to use TAP at all - IMHO we wanted to go for some standardized format. Although using embedded YAML is not IMHO really messing with the format (it's a part of the TAP13 protocol http://podwiki.hexten.net/TAP/TAP13.html?page=TAP13), I see where you are going with this. And to be honest, I do agree. Since most of the data we are (at least as far as I can tell) going to use will be in the embedded YAML, we can probably equally easily just create the same structure in pure YAML. And it does not necessarily need to look that different from what TAP looks like (I'm not saying that it needs to look like it, though).

I'm all for using a standard format but adding to what you're saying - what part of the TAP standard are we really using. We're already looking at implementing a TAP13 parser and I'm just not sure that we're doing something more than using the standard mostly in name only.

As much as the actual idea is flawed (IMHO), it corresponds with my idea of the ResultsDBs role - it should be storing the information, that enables us to query and aggregate the results of tests for specific 'testable'. E.G. for the update gating, we need stuff like ENVR and arch. On top of that (if the information is present at the time of the check-run) we might want to have the update-id stored, ... When gating updates, we can then have an intermediatory layer, that (when given update id/title) will know what our tests do, and what kind of information is stored in ResultsDB (let's say that Upgradepath results are stored per-build, Depcheck is stored per-update). That middleware (?) would then query Bodhi, to get the list of builds for the specific update, query ResultsDB for the Depcheck's result of the update, the results of Upgradepath for each build, and then will aggregate the resuls (e.g. all builds need to be tested, and all tests need to be passed), and based on the data, it would say "gate/don't gate".

My recollection from our conversations at Flock is that we decided to minimize the work that went into the flask-based ResultsDB so that we'd have something that worked. Once the other moving parts were more solid, we would go back and figure out what all we needed from a results storage system.

I think that it's still a bit premature to be looking towards update/build gating since we haven't really specified the middleware yet. Especially when we're already 2 weeks late and the new resultsdb system has yet to be tested in production. I'd really like to see some basic implementation soon, even if it doesn't end up being final.

What I'd like to solve (and I'm not sure how to do it "pretty") is giving the check-developer a way to specify "These parts of the output (or these key-value pairs), I want to have stored for later querying." Specifying some standard set of keys would be great. E.g. outcome for "how it ended", tested storing the identifier of what was tested (envr, iso name, repo url, whatever we decide to test), arch as a standard key for storing architecture, and quite probably a few more, that I can't think of right now.

While I see what you're getting at, I'm also pretty sure that whatever we come up with will need to be changed before too long. I think it highly unlikely that the initial implementation of the resultsdb reporter will be perfect even if the backing resultsdb doesn't change for a while. Again, unless I'm misunderstanding or mis-remembering something, we're going to be reworking resultsdb in the mid-term future anyways and I'm not sure we want to go too far with customization right now.

Which would then store something like this in the resultsdb store:

I'm not understanding how multiple builds in the same update would be represented. Would 'tested' be a list or builds or would there be multiple 'result_data' objects in the same entry? Would log files for each build be supported?

I'm all for using a standard format but adding to what you're saying - what part of the TAP standard are we really using. We're already looking at implementing a TAP13 parser and I'm just not sure that we're doing something more than using the standard mostly in name only.

I'm thinking about this again and am wondering - how many languages have TAP13 generation libraries that include the embedded yaml?

If the question is whether to use a standard format (TAP13) or a custom format (pure YAML), and none of the solutions have any obvious advantages over the other, I'd vote for a standardized format. It's healthier for the ecosystem. If we end up writing a parser, it's better to create a standards-based parser that can be published as a separate tool, rather than just implement our own stuff. Also, as time passes, other languages might get these parsers without our contributions. That's not going to happen for a custom format.

After some long conversation with @kparal, we came to the conclusion that the check-output (be it TAP, YAML or whatever else) needs to have at least some basic level of "added semantics". This is not only important for future reference (e.g. was libmodplug-0.8.8.5-1.fc19 build or update?), but also for our reporting.

In the bright future of FedMSG, we will need to be able to spawn the right message types without the need of additional information specifying "results of test named XYZ are for ISOs, FOOBAR is for builds...", etc. Also, since this information about the type will be stored also in ResultsDB (more on this later), we will be able to re-generate the FedMSG messages, if we ever need to.

This (partly) came up as a result of a discussion about the Upgradepath's TAP output - since upgradepath reports results both per-build (when an update might not even exist), and per-update. To avoid the need of having separated build/update upgradepaths, or hardcoding some "internal knowledge" about the results meanings into the reporting directives, we can just add a "type" (name is not yet decided, feel free to come up with ideas) key-value pair into the YAML data.

This means that we will have three "reserved words": tested_item, tested_type, and logs (names are a subject for further discussion change). The initial list of possible values (which means "values that we will act on" not "if some other value is filled in, we crash") is: bodhi-update, koji-build, possibly also iso or rpm, who knows.

The immediate gain is, that the Bodhi-Comment directive can then easily just go through all the data provided in the TAP output, check each entry for the presence of tested_type = bodhi_update keyval pair, and safely ignore the rest. Thus (e.g.) upgradepath can safely produce something like this:

ok - bar-1.2-3.fc20  # PASS upgradepath for build bar-1.2-3.fc20
  ---
  tested_item: bar-1.2-3.fc20
  tested_type: koji_build
  ...
not ok - foo-1.2-3.fc20  # FAIL upgradepath for build foo-1.2-3.fc20
  ---
  tested_item: foo-1.2-3.fc20
  tested_type: koji_build
  ...
not ok - foo-1.2-3.fc20, bar-1.2-3.fc20  # FAIL upgradepath for update foo-1.2-3.fc20, bar-1.2-3.fc20
  ---
  tested_item: foo-1.2-3.fc20, bar-1.2-3.fc20
  tested_type: bodhi_update
  ...

And the bodhi-comment directive will (in this particular case) ignore the per-build results, and report the update-related result. ResultsDB will just store all of it.

Just to put this all in one place, and in proper TAP13:

Keys with special meaning:
tested_item - identifies what was tested (e.g. bar-1.2-3.fc20)
tested_type - adds additional information, which describes whether the tested_item was iso, build, update or something completely different (currently proposed values are koji_build and bodhi_update)
outcome - provides an override to the standard ok and not ok of the TAP output.
summary - provides an override to the TAP's description part (or maybe, if the summary is not provided, we just won't store it, whatever makes more sense...)
logs - section that is excluded from the ResultsDB reporting

TAP version 13 
1..3
ok upgradepath for build bar-1.2-3.fc20
  ---
  tested_item: bar-1.2-3.fc20
  tested_type: koji_build
  ...
not ok upgradepath for build foo-1.2-3.fc20
  ---
  outcome: NEEDS_INSPECTION
  tested_item: foo-1.2-3.fc20
  tested_type: koji_build
  logs:
     errors: bla bla bla
     warnings: foo bar cow
  ...
not ok upgradepath for update foo-1.2-3.fc20, bar-1.2-3.fc20
  ---
  outcome: NEEDS_INSPECTION
  tested_item: foo-1.2-3.fc20, bar-1.2-3.fc20
  tested_type: bodhi_update
  summary: "1 PASS, 1 NEEDS_INSPECTION"
  ...

Taking the second one as an example, the respective ResultsDB's store-request would be:
{ "outcome": 'NEEDS_INSPECTION', "testcase_name": 'upgradepath', "summary": 'not ok upgradepath for build foo-1.2-3.fc20' "result_data": { 'tested_item': 'foo-1.2-3.fc20', 'tested_type': 'koji_build', }, "job_id": int, "log_url": string }

This means, that we have reasonable way to decide what steps to take on the reporting part - be it producing the right kind of Fedmsg notification based on the tested_type, or deciding which "results" should be added as bodhi-comments. We also have the data stored in the resutls storage (namely resultsdb), so we can possibly recreate the messages, and/or give the results some additional kind of meaning.

On another note, we also need to find a way to create/store the logfiles i.e. not the TAP output, but short/full logs, and creating URL's pointig to them. But this is another issue to solve, and is not (necessarily directly) tied to what we are trying to achieve in this ticket.

Overall, this makes sense to me.

tested_item - identifies what was tested (e.g. bar-1.2-3.fc20)
tested_type - adds additional information, which describes whether the tested_item was iso, build, update or something completely different (currently proposed values are koji_build and bodhi_update)

Why not just call these 'item' and 'type'? I'd prefer to avoid using 'test' here since we're trying to stick with the 'check' terminology. Either way, I don't see how having the longer key helps us here.

logs - section that is excluded from the ResultsDB reporting

Maybe something like 'output' or 'details' would be a better key for this? I see what you're getting at but I think that having links to logs and a 'logs' key would be confusing.

What would nested messages look like here? I'm thinking about depcheck output where results will be in the form of updates but it might be a single build in an update which failed. Is the idea to just stuff all that into the 'logs' key?

Those are good points, Tim. Thanks! item and type do sound much better, so let's stick with that.
From the output/details I'd be more inclined to choose the details, as it IMHO is less inciting to putting all the output there.

I have not yet given that much thought to the nested messages, but it it is something that "makes sense for the person reviewing the result" (IIUIC), then I'd either stick it into the details section, or into the log file. As each result can point to a different log file, it makes sense (for me) to have the "detailed info" in the logfile, and then just give the (maybe even html-pretty-printed, as we did in AutoQA) to the user via link.

Thoughts?

From the output/details I'd be more inclined to choose the details, as it IMHO is less inciting to putting all the output there.

Sounds good to me.

I have not yet given that much thought to the nested messages, but it it is something that "makes sense for the person reviewing the result" (IIUIC), then I'd either stick it into the details section, or into the log file. As each result can point to a different log file, it makes sense (for me) to have the "detailed info" in the logfile, and then just give the (maybe even html-pretty-printed, as we did in AutoQA) to the user via link.

That works for now. I agree that we may want to look into the kind of pretty reports that we did for depcheck in autoqa but that's something we can address once stuff is actually running :)