[PATCH 04/10] models: Add 'status' model

[Finucane, Stephen]

> On Wed, Sep 23, 2015 at 08:04:26PM +0100, Finucane, Stephen wrote:
> > * Context is a non-unique name used to distinguish different tests (in
> > the broader sense). Examples would include: intel/license-check,
> > intel/unit-tests, dpdkorg/unit-tests/module1,
> > dpdkorg/unit-tests/module2.
> >
> > Is that clearer? I'm sorry but there is a limit of how much
> > information I can include in the models before messing up the 'admin'
> > display. I plan to add developer documentation in a follow up patch.
> >
> > > What I'm assuming is that a lot of the "Status" rows will contain the
> > > same information, because it really belongs to a Test object. We're not
> > > just manipulating TestResults but Tests as well, so we need to describe
> > > those objects as well.
> 
> So it does mean my assumption was correct, eg. 'intel/license-check'
> will be repeated for every patch. Hence abstracting a Test table we can
> use for good.

Yes, but the context does not imply that the tests are the same. Please refer to the GitHub documentation for this. The non-uniqueness of this is important.

https://developer.github.com/v3/repos/statuses/

> > > Another design question: How to you know with your model that all tests
> > > have been run? That's needed to determine that, indeed, that patch has
> > > passed all the defined tests (a "test" can of course hide a full test
> > > suite, summarized in one entry in patchwork).
> >
> > Ah - I think you are misunderstanding what a "Status" represents here.
> > A status does not represent the status of test that *has* to be run.
> > It is a way of reporting the status of *any* tests that *have* run.
> 
> 'pending' suggests otherwise.

*have been run or are currently running

> > Patchwork doesn't care what tests, if any, have been run: it is only
> > an aggregator for results. This makes sense as patchwork is not the
> > place to be storing configuration for or enforcing the tests that have
> > to be run: all this management should be done using existing tools and
> > mailing lists.
> 
> Why not? I don't see anything that would prevent the admins from
> defining the list of tests we are expecting results for a certain
> project. We can do interesting things with that information, like making
> sure we've run all the tests for a patch and only send the test results
> back to the ml with an "ok/ko" mail + links.

This stuff is already happening outside of patchwork on the 'report' mailing list. See the original proposal here:

https://lists.ozlabs.org/pipermail/patchwork/2015-July/001363.html

If you wanted a cumulative result, you can do this outside of patchwork. This allows for the "different modules different tests" scenario.

> This doesn't go against patchwork being just a test result aggregator,
> quite the opposite, I do agree there FWIW.

Yes, it does. It means you're using patchwork to dictate what tests are allowed. Please provide an example of another reporter that allows you to do this.

You are also assuming the "admins" for patchwork are the ones who would decide what tests are run. This is a dangerous assumption.

Finally, how would you deal with the case where changes to different files would result in different tests?

> > This also allows smarts on the CI system whereby different tests can
> > be run for different types of patches (a change to files in the 'docs'
> > folder would run different tests than a change to 'src'). In summary,
> > it is not necessary to define a distinction between tests and test
> > results because we only care about the latter.  Comprende? :)
> 
> I don't think that a compelling argument. For instance, in that case, we
> could have a more explicit "skip" or "non necessary" state for tests not
> run and have more visibility on what the testing system actually does
> while still tracking testing completion.

Again, patchwork should not be in charge of this. A developer or developer-provided script decides what tests to run. Patchwork just reports.

> And si, comprendo, no soy estupido.

I wasn't calling you stupid - I was trying to be friendly. Cop on.

> > > > One point regarding the "SeriesTestResult": I had planned to
> implement
> > > > this as a "lowest common denominator" property. That is, if a patch
> in
> > > > a series fail, the series is marked as fail etc. What you describe (a
> > > > unique, per series property) is quite a limited use case but its
> > > > behavior could be emulated by only testing the last patch in a
> series.
> > > > This would require very little "smarts" on your client end.
> > >
> > > I don't think introducing special cases is a good thing, eg. how will
> > > you really differenciate the case of a test run on a full series and a
> > > test run on the last patch? you'll have to look at the other patches to
> > > see if a test run was run on them as well?
> >
> > You're right. What you'd actually need to do is test the last patch
> > and set the status on all patches.
> 
> A different special case, but still a special case. I can totally see an
> improvement to run a test on the full series as the first step and only
> do per-patch testing when the full series fails to narrow down the
> faulty patch(es). Sure you can then "override" the test result
> per-patch, but we're just having special case after special case. Not a
> good start IMHO.

You're describing ZUUL, the tool that OpenStack use to do parallel testing. We're not going to duplicate this work.

I'm following the proposal by Thomas Monjalon of 6WIND. This series works for that proposed model while yours does not. This has been agreed on the mailing list. If you have issues, I suggest taking it up in the original thread. Otherwise, can we please review this given in light of that proposal?

https://lists.ozlabs.org/pipermail/patchwork/2015-July/001363.html 

> --
> Damien