Bug 11580 - oe-selftest: blocking buildhistory enablement is too onerous
Summary: oe-selftest: blocking buildhistory enablement is too onerous
Status: RESOLVED WONTFIX
Alias: None
Product: Build Testing
Classification: QA/Testing
Component: general (show other bugs)
Version: unspecified
Hardware: x86 Multiple
: Medium+ normal
Target Milestone: 2.5 M2
Assignee: Leonardo Sandoval Gonzalez
QA Contact:
URL:
Whiteboard:
Depends on:
Blocks:
 
Reported: 2017-05-25 03:58 UTC by Paul Eggleton
Modified: 2017-11-01 20:08 UTC (History)
3 users (show)

See Also:
OS type for building Yocto: ---
Type of Regression: Regression (Used to work)
Verified:
Documentation change: No (bug/feature does not impact docs)


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Eggleton 2017-05-25 03:58:36 UTC
oe-selftest currently fails if you have buildhistory enabled. As I always tend to have buildhistory enabled I think this is quite an annoying restriction, and we shouldn't be putting roadblocks in front of users running oe-selftest. I understand the reason we disabled it in 41fb482ee132289e8663c76b813a6e3c86e201b3 was to avoid triggering warnings and thus failing tests, but surely we could just remove version-going-backwards from ERROR_QA / WARN_QA in configuration for those tests? I see we already do just that in the sstate tests.
Comment 1 Leonardo Sandoval Gonzalez 2017-05-30 17:18:17 UTC
(In reply to comment #0)
> oe-selftest currently fails if you have buildhistory enabled. As I always
> tend to have buildhistory enabled I think this is quite an annoying
> restriction, and we shouldn't be putting roadblocks in front of users
> running oe-selftest. I understand the reason we disabled it in
> 41fb482ee132289e8663c76b813a6e3c86e201b3 was to avoid triggering warnings
> and thus failing tests, but surely we could just remove
> version-going-backwards from ERROR_QA / WARN_QA in configuration for those
> tests? I see we already do just that in the sstate tests.

Jumped into this today and I wonder what would be the best approach: 1. ignore the version-going-backwards for all test cases (meaning that we include it in the setUp base class) or 2. place the ignore statement per test case. In my opinion, the safest would be to include it per test case, option 2 (per unit test is quite granular, I believe) this way we do not ignore an important QA check for all tests. I am testing both approaches.
Comment 2 Richard Purdie 2017-05-30 18:08:14 UTC
Given the different codepaths that buildhistory exercises, I'd really prefer that its either always on or always off as otherwise oe-selftest isn't deterministic.

I did originally look into disabling specific tests however I seem to remember running into other issues, sadly I don't remember what those were :(. Add in the determinism argument above and I decided it needed to be off, the tests are about other things, not about buildhistory which has its own specific tests.

As I alluded to at the time, I'm happy enough for the code to disable buildhistory, I'm not sure there is an each way to forcibly "uninherit" a class though and simply setting some buildhistory disable flag doesn't really seem deterministic to me either.
Comment 3 Paul Eggleton 2017-05-30 22:01:39 UTC
Another alternative would be to error out only if buildhistory is enabled *and* version-going-backwards is in ERROR_QA / WARN_QA. That still presents a bit of a roadblock by default but at least there'd be a way to keep buildhistory enabled.
Comment 5 Leonardo Sandoval Gonzalez 2017-06-14 17:09:36 UTC
Patch sent but not merged yet. M1 cut-off is in place, so moving to M2
Comment 6 Richard Purdie 2017-06-22 10:27:36 UTC
Paul and I have talked about this and we disagree on how we should solve this. I do understand that we need to support running oe-selftest from an environment where buildhistory is enabled and I agree with that. Where we disagree is how we should do it.

buildhistory is invasive and has side effects for inclusion of the class. What I worry about is test determinism. If for example the code in buildhistory which stops it changing task signatures breaks, should just the buildhistory test for this signature independence break, or should some random set of tests break making it harder to understand where the issue was?

Looking at the code, we have three 'error' cases right now, buildhistory, prserv and sanity_tested_distros. We have two basic options:

a) filter out known bad settings

b) create an environment where we only filter though "known good" settings.

For a), it gets tricky as you could add INHERIT_remove = "buildhistory" but that means the buildhistory tests are going to struggle to enable it again. We could create a dummy empty buildhistory class but that would break "inherits(buildhistory)" type checks. The user (or distro) may also set buildhistory somewhere other than INHERIT_DISTRO. We therefore can't likely filter out all known bad settings, just make a strong attempt. We can keep the existing check to confirm we were successful at removing it.

For b), in order to support parallel execution of oe-selftest unitests know we're going to have to support multiple build directories. If we have to do that anyway, I have wondered about whether we could filter the config files in the process. To quote Paul, "eSDK code uses bb.utils.edit_metadata() - that does require a callback to be written that makes the changes" so it could be possible. Knowning which variables to filter is tricky, e.g. what if the DISTRO inherits buildhistory? We want to run some of the sstate sig tests with the user's wider layer configuration and potentially their distro settings.

All things considered, this is going to need further discussion/thought. This hopefully summarises some of the pros/cons/issues though.
Comment 7 Richard Purdie 2017-06-23 08:30:09 UTC
Just as another note, 

ERROR_QA_remove = "version-going-backwards"
WARN_QA_remove = "version-going-backwards"

doesn't work since we can't then enable this check and the test cases test_buildhistory_buildtime_pr_backwards and test_buildhistory_diff then fail.
Comment 8 Leonardo Sandoval Gonzalez 2017-07-19 15:29:38 UTC
Based on RP comments, a new revision needs to be created. Moving to next release.
Comment 9 Leonardo Sandoval Gonzalez 2017-09-13 20:56:51 UTC
Discussion is ongoing so revisiting this on 2.5.
Comment 10 Paul Eggleton 2017-11-01 20:08:03 UTC
I'm closing this for now as it's not really practical to fix; I'll just live with it.