Robottelo: @skip_if_bz_bug_open logic should involve version check

[rplevka]

since we support multiple versions of katello (satellite), and not all bug fixes are being backported to all versions, the following scenario might (and most probably will) occur:

1.) A nasty bug is discovered and reported on sat-6.2.2
2.) An appropriate test with @skip_if_bz_bug_open('123456') is being created and pushed to master, and 6.2.z branches.
3.) Devels mark the target version as sat-6.3, deliver a fix and close the BZ.
4.) @skip_if_bz_bug_open no longer skips the bug.
5.) test passes on 6.3.* but starts to fail on 6.2.z as the fix hasn't been (and is never going to be) backported.

Shall we start deleteing such test cases from version branches which are not supported, or at least start marking them as @stubbed or shall we implement some version check mechanism (check the BZ flags, etc.) to the BZ decorators?

discuss ;)

[omaciel]

Great point @rplevka ! I need to think a bit now :)

[elyezer]

That is a really good point!

[lpramuk]

Blocking issue is #3896 as there is _wrong logic_ in bz_bug_is_open()

Lets have this fix on top of that fix ;-)

[rplevka]

i'll keep the list of tests (which should be updated once version-check feature is implemented) here:

| branch | BZ# | test case | comment |
| --- | --- | --- | --- |
| 6.1.z | 1138553 | cli.test_user.UserTestCase.test_positive_remove_role | |
| | | cli.test_user.UserTestCase.test_positive_add_role | |
| 6.1.z, 6.2.z | 1221971 | ui.test_adusergroup.ActiveDirectoryUserGroupTestCase.test_positive_update_external_roles | |
| 6.1.z | 1151220 | api.test_architecture.ArchitectureTestCase.test_positive_post_hash | |
| 6.1.z | 1328092 | test_positive_synchronize_auth_yum_repo | add decorator |
| | | test_positive_synchronize_auth_puppet_repo | add decorator |
| 6.1.z, 6.2.z | 1302725 | api.test_bookmarks.BookmarkTestCase.test_negative_create_null_public | |
| 6.1.z, 6.2.z | 1328935 | ui.test_operatingsystem.OperatingSystemTestCase.test_negative_create_with_too_long_description | |
| 6.1.z, 6.2.z | 1357864 | cli.test_classparameters.SmartClassParametersTestCase.test_positive_puppet_default | |
| 6.2.z | 1349364 | api.test_discoveredhost.DiscoveryTestCase.test_positive_upload_facts | |
| 6.2.z | 1343392 | cli.test_host.HostUpdateTestCase.test_positive_update_name_by_id, cli.test_host.HostUpdateTestCase.test_positive_update_name_by_name | |
| 6.2.z | 1265150 | ui.test_hw_model.HardwareModelTestCase.test_positive_delete | |

[abalakh]

Bumping priority. Another bug which landed 6.3 is 1324508, and as it's not in 6.2 we've got 22 extra failures (11 per rhel version) for 6.2.z - basically all ui bookmarks tests are failing as the bug is striking at test setup.

[rplevka]

I was also thinking of implementing a new kind of decorator: skip_wontfix. Or we could integrate that scenario in the skip_if_bug_open for cases where there are flags for particular version missing

[elyezer]

One of the configuration Pulp Smash receive is the expected version of the system, that allows:

• Make sure the system under testing reports the right version number
• Skip bugs based on the version
• Run or skip tests based on the version

Worth noting that Pulp Smash does not have helper classes like Robottelo does, for example there is no CLI class wrapper based on entity. All commands are run on the test directory (we do have functions to avoid repeating same chunks of code over and over again). Also we didn't faced any API or CLI changes yet.

With all that said I think one thing that robottelo could have is a config with the version and then make the bug skipping logic to consider that information when checking if the test should be skipped or not.

[rplevka]

@elyezer, the version is given by the robottelo branch, so this is out of scope. The problem is on the other side of things - Bugzilla. Since dev folks haven't cloned the BZ's for each version they'd like to address.
Currently, I see the approach of having a BZ for every applicable sat version as the most plausible one.

[elyezer]

I prefer a more automated approach, adding the version information is very easy and automation can take care of it, managing many BZs is harder and will duplicate a lot of stuff.

[rplevka]

@elyezer i still can't see how this addresses the issue.

[elyezer]

For example bug https://bugzilla.redhat.com/show_bug.cgi?id=1325995, it has been verified on sat 6.3 (its flags is where the information should be checked). The decorator having the version information can check if it is close and is on the expected version, in this case, sat 6.3 and verified, then if it matches then the test will run, otherwise it is going to be skipped.

[rplevka]

@elyezer yes, i got you there, but the satelltie version is the bit i have problem with - what's the use of 'automatically' finding out the satellite version if our version scope is limited by the branch we're in?
our plan was to keep the version in constants, since that's not a subject to change.

[elyezer]

@rplevka we can do that way, but the version stated by the system can be wrong. I am saying that because every release or version change we have to open a new bug to fix that.

Basically the premise is: never rely 100% on the system you are testing as it could provide wrong or unexpected values.

[rplevka]

@elyezer, ha! that's where the misunderstanding comes from.
No, we're not parsing the version from a system. We have the satellite version hardcoded in robotello per branch.
Basically, when we run robotello/6.9.3 we simply treat the satellite as 6.9.3. We don't do any checks to modify the behaviour and I think it is a right thing

[elyezer]

Ok, the version is on git, robottelo needs to know that so it can make the bz skip decorator check for the proper version. I think the best way to do is to expect it as a configuration. We have branches for y releases (6.1, 6.2 and 6.3) but an specific bug can be handled on a z release, for example something is broken on 6.2.2 and got fixed on 6.2.3 so a particular test for that issue should run on 6.2.3 but not on 6.2.2.

Automation pipeline can fill that information and better skip/run logic can be created.

[rplevka]

@elyezer, our robotello branches are not for Y-branches, but Z-branches (6.2 branch means the recent 6.2.*) - that's the common misunderstanding. We only test the newest Z-version as only that one is supported (the one on CDN).

[elyezer]

@rplevka what I said is if robottelo receive the satellite version as a configuration then we can even distinguish between z-streams. I am not talking about git at all, even though we have the version as branch in git I am not considering it since I think robottelo needs a configuration for the version, same configuration that the bz skipping should consult when deciding to skip a test or not.

A prototype to what I am saying should be something like:

robottelo.properties:

[server]
...
version=6.2.3
...

robottelo.config

from packaging.version import Version

class ServerSettings(...):
    def read(self, reader):
        # ...
        self.version = Version(self.reader.get('server', 'version')

robottelo.decorators

from packaging.version import Version

def bz_bug_is_open(bug_id):
    # ...
    if bug is None or bug.status not in BZ_OPEN_STATUSES:
        if settings.server.version < Version(bug.version):
            # Treat bugs closed for lower versions as being open is order to skip tests
            return True
    # ...

[rplevka]

@elyezer ah, then we actually share our opinions. Yes, that's pretty much what i meant when i said

No, we're not parsing the version from a system. We have the satellite version hardcoded in robotello per branch.

I would just stick to 6.2 instead of 6.2.3 as that would mean we should bump the version for every snap we receive.

[elyezer]

The version should be passed by the automation and not hard coded, automation knows what version to use and also this is something that can vary depending on the satellite version under testing. I may want to go back and test a previous version for example.

This is my suggestion, if the Y release is enough then that is ok and the version can be hard coded. In long term something can be discovered on 6.2.3 for example and just get fixed at 6.2.4 where the 6.2.3 is the current GA and 6.2.4 is a testing build.

[rplevka]

sorry, my comment on CDN was a little misleading. My point was, we always test the latest stuff we have (testing build). and we run it through the 6.2 pipeline. once there's 6.2.4 testing build, we're no longer interested in 6.2.3

[elyezer]

@rplevka so the version can be hard coded into the properties file.

[svtkachenko]

Bump. Recently @omaciel, @abalakh, @ldjebran and I had a discussion regarding such huge list of 'fixed in 6.3 only` bugs and affected tests. And we stick to the same decision -- automate version checking.
However, looks like devs using version flags not at the same fashion as we do. @omaciel should clarify it with devs.

[rplevka]

@svtkachenko I would simply start cloning the Bugzillas for the individual branches (which is btw what devs are planning to do)

[abalakh]

@rplevka what if devs won't fix those bug for 6.2 (e.g. have no time for that, especially if issues have low priority)? they'll close the bugs with 'wontfix' resolution and tests will keep failing.
also, how will me manage the bugs in the next releases? will clone bugs for 6.4, 6.3, 6.2 when 6.5 comes out, for example? i know it's a bad example 'cause we probably won't support all the versions, but still

[rplevka]

@abalakh we could deal with such cases by implementing skip_wont_fix (or simply skip) decorator

for the version support, as far as i know, only last 2 versions are supported.

@svtkachenko the problem on version checking is that a single bugzilla status can't reflect the actual status of multiple versions. Due to this limitation a new BZ needs to be cloned for each reported version.
I totally feel your pain, I also went down that path :)

edit: the won't fix status can actually by incorporated directly into the skip_if_bz_bug_open logic - it would be skipped with "skipping due bugzilla closed as won't fix"

[omaciel]

Guys, wait until tomorrow for my proposal?

[abalakh]

@omaciel sure :)

[omaciel]

@abalakh check email with document I wrote about this topic :)

[renzon]

After playing some time with the API, I discovered "flags" is only available on authenticated calls. I opened a bug for it: https://bugzilla.redhat.com/show_bug.cgi?id=1399755. So I am going to add bugzilla credentials on robottelo.properties accordingly.

[rplevka]

@renzon I don't honestly think that is a bug. The same feature applies to comments. Unauthenticated call for comments fetches only those which are not marked as private. I believe same applies to flags, etc.

[renzon]

@rplevka you are right. But I think at least docs could be more explicit about it. The only place I found something is https://bugzilla.readthedocs.io/en/latest/api/core/v1/general.html#authentication ant that just after thinking by myself about the authentication. I would put a warning about it on every property, or at least on any page of API where this aplies: https://bugzilla.readthedocs.io/en/latest/api/core/v1/bug.html#get-bug.

[omaciel]

Let's make sure that Bugzilla credentials are optional.

[renzon]

@omaciel without credentials we cant get flags data. What should we do in this case: skip test or not? I think we should run target test in this case, but want to be sure.

[rochacbruno]

If we cant communicate to BugZilla then I think all bugs should be "skipped due to bugzilla authentication problem"

That case will only happen when running tests locally and if it is a test case under development the dev can comment out the decorator to force the test to run. (easier then forcing skip of many of them)

[omaciel]

@renzon I agree with what @rochacbruno said: skip it

[renzon]

@rochacbruno not communicating is isn't what I had in mind. This case is already handled by the decorator now.

Once @omaciel mentioned credentials I was thinking on unauthenticated communication with Bugzilla. In fact we are doing this kind of communication because all public members exposed were enough to skip decision: bug status and whiteboard. But now we need flags to implement new BZ flow. In this case, if credentials are absent I cant read flags (in fact a receive a empty list for this parameter on unauthenticated calls).

So I am thinking on keeping current behavior if flags is empty, which by now is considering only status and whitebord but no flags. This can lead to false positives. But once our automation is already prepared for this, we will not have false positives on jenkins jobs what is the very motivation of this issue.

Anyway I put my own bz credentials on robottello.properties to test and every qe dev can do the same. My intention is sending an email for list stating tha so those who want skip behaviour considering flags locally can have it. What you guys think?

[elyezer]

So I am thinking on keeping current behavior if flags is empty, which by now is considering only status and whitebord but no flags.

:+1:

The skip message can be adjusted in this case stating that it was skipped without the flag information.