Robottelo: Remove Robozilla dependency on skip_if_bug_open decorator

[rochacbruno]

For now to be applied only on master branch.

PR 1: https://github.com/SatelliteQE/robottelo/pull/7346

• [x] Explicitly mark wontfixed test cases with @pytest.mark.deselect without calling Bugzilla and remove all the calls for bugzilla in the test collection phase. we want to keep the tests in codebase for historical reasons.
• [x] Identify tests that can run (passing tests) and remove the blocker.
• [x] Replace @skip_if_bug_open with explicit @pytest.mark.skip(reason="BZ:123456")
• [x] Remove direct dependency on Robozilla for decorators and remove bugzilla calls.

PR 2: Done! https://gitlab.{internal-url}/satelliteqe/nagger_bz

• [x] Crate a nagger script that parses that BZ numbers and notifies component owners if a test is skipped by an already closed BZ and ask for update.

PR3: IN progress: https://gitlab.{internal_url}/satelliteqe/nagger_bz/blob/master/DOCS.md

• [ ] Update the documentation on how to use the decorators and strategies for BZ based workarounds.

[rochacbruno]

@pgagne @flatfender @jyejare @sghai @mirzal

This is the plan and I am in progress on the item 2, so I'll send the first PR soon.

If you have any comment.

[mirekdlugosz]

• Crate a nagger script that parses that BZ numbers and notifies component owners if a test is skipped by an already closed BZ and ask for update.

I understand it would take BZ number from new decorator (@pytest.mark.skip(reason="BZ:123456")), is that correct?

The script will send emails based on

• last person that made changes on that line?
• component owner mapping?
• CODEOWNERS file
• BZ qa-contact?
• A single email to the list

I'm undecided between "component owner mapping" and "BZ qa-contact".
On one hand, component owners should own their component, which sometimes mean they need to take care of bugzillas created by other QE. On the other hand, BZ qa-contact is the best person to make any decisions regarding specific bz and any automation around it. Most of the time component owner and BZ qa-contact will be the same person, and if they differ, then probably there is a good reason for that.

But then, there might be rare cases when BZ qa-contact is unresponsive (left the team or something), while component owners are always up-to-date.

Perhaps we can do both?

Either way, I vote for one single email to list, possibly CC-ing specific people (these that have cases with resolved bzs). This way it will be more visible and if anyone has free time, they can take offload someone with a lot of bzs.

[rochacbruno]

I'm undecided between "component owner mapping" and "BZ qa-contact".

@mirzal I think we can do both, my next question is: "Where do I find the component owners mapping?" do we have a file that maps stuff like:

{
    "pulp": ["kersommoura", "rochacbruno", "bherring"],
    "Repositories": ["user1", "user2"],
    ...
}

We need to build a mapping like that for the script, internally on gitlab.

[mirekdlugosz]

@rochacbruno there won't be mapping file on gitlab, as this approach doesn't fit some of management workflows.

Instead, we have Python library responsible for gathering component owners data from bugzilla and Mojo, and thin command line wrapper that creates YAML file for further processing. At the moment they live only in my private forks and are not integrated with CI. Since your nagger script will be one of the consumers here, I welcome all suggestions on design of such integration.

Library: $SAT_GITLAB/satelliteqe/satreportinglibs/tree/master/satreportinglibs/componentowners
CLI wrapper: $SAT_GITLAB/mzalewsk/satellite6-reporting/blob/rework-extract-libs/component-owners/component-owners-manager

Wrapper will produce YAML file with content like that:

API - Content:
    full-name: API - Content
    primary: jyejare
    secondary: mzalewsk
    slug: api-content
Activation Keys:
    full-name: Activation Keys
    primary: pgagne
    secondary: swadeley
    slug: activationkeys

However, I am not yet sure how we will transfer it between jobs.

You can also import library to get dict with the same data, like that:

from satreportinglibs.componentowners import ComponentOwners
mapping = ComponentOwners.as_dict()

For this to work, you need environment variables with Mojo username and password, or run your script on the same machine that first created YAML - thanks to request response caching, you will get the same data without knowing Mojo credentials.

[jyejare]

@rochacbruno, The plan in phases is a good idea. you++ :)

I have some questions:

1. Are you planning to merge all phases once or sequential?
2. We are OK with nagger script but can we un-skip the test if BZ is verified(just like we do it now) else the skipped test has to wait until the component owner removes the skip explicitly.

We wanted to implement this functionality just to save the calling time from test to Bugzilla. The solution should not impact the existing way of skipping/running test based on Bug Status.

We can manipulate pytest.mark.deselect(instead of skip) based on the function(we need to implement) that returns True / False wrt bug status.

[rochacbruno]

Are you planning to merge all phases once or sequential?

Sequential, #7346 is the 1/3 I am addressing the requested changes right now and hopefully we can run a CI against it and merge soon

We are OK with nagger script but can we un-skip the test if BZ is verified(just like we do it now) else the skipped test has to wait until the component owner removes the skip explicitly.

To un-skip the test the component owner has to remove the .skip or the .deselect mark, only after this is done and PR merged the test will run again.

We wanted to implement this functionality just to save the calling time from test to Bugzilla. The solution should not impact the existing way of skipping/running test based on Bug Status.

Not sure I understood this statement, we are keeping the deselection of wontfix marked tests, that before was done on pre-selection and now is explicit using .deselect and the skip will be the same but now it is explicit and no more conditional by BZ.

We can manipulate pytest.mark.deselect(instead of skip) based on the function(we need to implement) that returns True / False wrt bug status.

This PR already manipulates the .deselect mark to remove from test collection, you are suggesting we capture the skip mark instead? and then do it conditional based on what data?

NOTE: deselect is to remove tests from collection, they don't even count on the report, the .skip is to explicitly skip and count on reports.

[pondrejk]

Hi @rochacbruno, sorry for being late to the party. From the discussion it seems to me that bz_bug_is_open is understood mainly as a way to work around temporary issues. But what about the use case of automating issues that weren't fixed yet (which the core of the colsed loop initiative). Perhaps you'll help me to understand how the new way covers this workflow:

1. I have a new BZ with qa_test_coverage set to ?
2. I asses the cause of the issue can be verified by adding an assert to an existing testcase.
3. I add the assert enclosed in if not bz_bug_is_open(BZ):
4. Once the BZ gets to ON_QA and lands on my table, I check the latest automation run.
5. Test passed, I mark bz as verified. I remove the bz_bug_is_open from case and add the bz number to the docstring

For this case the bz_bug_is_open was a powerfull tool, and I'm not sure the new setup would cover it this efficiently, or?

[rochacbruno]

Hi @pondrejk in the new way we will rely on the notification script for this.

1. I have a new BZ with qa_test_coverage set to ?
2. I asses the cause of the issue can be verified by adding an assert to an existing testcase.
3. I add the BZ number to the test docstring
4. I add the assertion commented out enclosed by a BEGIN:END block
   # BEGIN BZ:123456 # assert something_that_depends_on_BZ # END BZ:123456
5. Once the BZ gets to ON_QA and lands on my table, the daily nagger script will send me an email notifying me that I can now remove my commented out assertion (the message will include the name of test, filename, line number), this strategy at the end has the same effect as it was an if but forces me to immediately take an action after I receive the notification.
6. Test passed and I do not have to do anything more.

[pondrejk]

Hm, the disadvantage of the above is that I have to un-comment the assert to make it run for the first time. With weekly snap cadence, when I get nagged it is probably too late, so I have to wait a week for next run or trigger the test locally.

Now let me complicate it a bit. If the bug was not fixed properly, and I have to set it to failed_qa, should I put the comment back to make assert inactive? And then un-comment it again when the new fix arrives and I get nagged again? With the old style I had to take action just once for clean-up at the end.

To be honest, the process is kind of convoluted, actually discouraging me from using the automated verification, because it would be faster for me to verify the thing manually.

[rochacbruno]

@pondrejk understood your concerns, and that was the best approach we found out until now.

Our first goal as stated on the CleanUp: skip_if_bug_open / bz_bug_is_open decorator (July 1st)

 we can save a good chunk of time by eliminating two decorators from Robottelo (wherever applicable):

    skip_if_bug_open
    bz_bug_is_open

So we dont have the bz_bug_is_open function anymore, because we want to get rid of the complex logic behind Robozilla. As we dont have this function anymore we dont have a way to do the conditionals.

The advantages are

• No more calls to Bugzilla (less time consumed by test runs)
• No need to maintain the complexity of Robozilla
• No more problems of hidden BZs
• More visibility as component owners will need to take care of nagger script notifications
• No more stale conditionals that is never cleaned up

Disadvantages are

• Will require a larger interval between the nagger notification and the change of the test case (that means we may have to wait a week or so or find a way to verify it using standalone automations)
• May require more changes on the test case if BZ changes status multiple times.

I think management is aware and ok with this as we discussed this on the email thread @flatfender @cswiii

anyway nothing is written in stone, we can go with this and if after one or 2 builds we find that this introduced a problem, we can still think in alternatives like implementing a new/better bz_is_open function.

[rplevka]

@rochacbruno

we can save a good chunk of time by eliminating two decorators from Robottelo (wherever applicable):

skip_if_bug_open
bz_bug_is_open

i think you misunderstood the original poster's message. If you read it again (especially the bottom message, where @jyejare sums it up, it literally reads:

So as said "wherever applicable" means, we can remove the decorators for BZs on tests those are closed.

• meaning, just doing the cleanup of the occurrences which are no longer needed due bug being closed for a long time.

I somewhat still don't get the benefit of degrading from automated process back to manual one. Failing to take immediate action (reviewing, modifying code, raising a PR and getting it merged in time) may cause noise in the test results which are directly reported.

One more thing is unclear to me - if we're doing all of this in order to get rid of the complicated logic of robozilla - how's the nagger script going to work? Isn't IT going to contain the same logic in order to correctly inform me about which tests might be un-tweaked?

[pondrejk]

anyway nothing is written in stone, we can go with this and if after one or 2 builds we find that this introduced a problem, we can still think in alternatives like implementing a new/better bz_is_open function.

Yes, and we should probably keep the old functions around. I think that would be a more humane way of deprecating things -- keep the possibility of using the legacy functions for cases when the new style doesn't fit. I mean no amount of discussion can cover all the problems that may arise in practice, and there's no need to make us more fragile by abruptly removing working tools.

[jyejare]

First of all, I would like to thank @rplevka for his comment https://github.com/SatelliteQE/robottelo/issues/7336#issuecomment-535687924 which clearly mirrors my expectations for this change. It's ok that we took one step more and 'invented' :) nagger script. But again I would point to the fact that we are diverting/degrading the benefits of having skip/unskip of tests/patches automatically through bz_bug_is_open and skip_if_bug_open (my second point in comment https://github.com/SatelliteQE/robottelo/issues/7336#issuecomment-532108953 .

Being in the world of 'everything automated', we are relying on nagger script which would slow down the entire process and would have an impact on overall test results (PQI) was not the intention for what I have started the thread.

The intention here was:

1. Remove the unused(the bugs which no more 'open') skip_if_bug_open / bz_bug_is_open decorators from robottelo.
2. There was no other plan, but somehow we decided to have a nagger email to component owners. But we don't want to hold the test running until the owner explicitly raises the PR to remove it(degradation of automation to manual). The nagger script would be just for notifying to remove the decorator that can save some seconds of time.

We also discussed Robozilla has performance issues but the solution was not to develop the entire architect behind it but improve Robozilla itself to suit our need.

@rochacbruno ,

To un-skip the test the component owner has to remove the .skip or the .deselect mark, only after this is done and PR merged the test will run again.

This is where / how we are degrading the use of automation by manually raising the PRs to unblock the test which is happening automatically now.

We wanted to implement this functionality just to save the calling time from test to Bugzilla. The solution should not impact the existing way of skipping/running test based on Bug Status.

Not sure I understood this statement, we are keeping the deselection of wontfix marked tests, that before was done on pre-selection and now is explicit using .deselect and the skip will be the same but now it is explicit and no more conditional by BZ.

I mean, keep the decorators as it and see how can we improve things from Robozilla or how we use robozilla. That is querying the bug statuses all in once and skip/unskip the test accordingly.

We can manipulate pytest.mark.deselect(instead of skip) based on the function(we need to implement) that returns True / False wrt bug status.

This PR already manipulates the .deselect mark to remove from test collection, you are suggesting we capture the skip mark instead? and then do it conditional based on what data?

I mean the deselection based on BZ status. This can be alternative of current skip_if_bug_open.

NOTE: deselect is to remove tests from collection, they don't even count on the report, the .skip is to explicitly skip and count on reports.

Yes I am known to it. But the deselection is faster and would save us the time.