Project

General

Profile

Actions

action #107824

closed

Modify is_shown everywhere it makes sense.

Added by JRivrain about 2 years ago. Updated 3 months ago.

Status:
Rejected
Priority:
Low
Assignee:
-
Target version:
-
Start date:
2022-03-03
Due date:
% Done:

0%

Estimated time:

Description

We saw that the solution applied here here in order to take more screenshots is working fine. We could probably apply it everywhere it makes sense with not too much work, even though the task may be a bit repetitive.
We determined some motivation for this and some possible problems in earlier discussions:

Problem:

But there is one thing that makes me sad: we no longer see what the module is doing, unless it fails (in which case we get logs and a screenshot). While not strictly necessary, I think it is nice to be able to visualize immediately what the module does without having to dig in the code. And that can be especially useful when we use very high-level methods in test module such as:

sub run {
    $testapi::distri->get_local_user()->import_existing_users();
}

In which case we'd have to dig in libraries to understand what it does, when in the past, with testapi, you would just have to look at pictures.

That can be specially useful for developers, to be able to visualize the steps to reproduce a bug.

One possible solution is already here:

To make sure we are on the right page, we implemented for each page a method called is_shown. Then in the controller, we have methods like "get_user_creation_page" which calls the "is_shown" method, and each method from the page is using "get_user_creation_page" hence each time calling is_shown. This would be in my opinion a good place to show "what is shown".

There are also some examples where review was made pretty difficult without this. For example here we would not even see that the pop-up is displayed. This was the reason why this modification of is_shown was made at the first place, as it was very tricky to figure out what was happening if something went wrong in the pop-up but then we would only see the main page at the end.
There are some places where this is not useful though, such as here: if only one action is done in the module that is visible in the sole screenshot at the end, in this case only click to activate.
In general, if the method/module used is descriptive enough and the sole screenshot taken at the end are good enough to understand all the actions done by the module, then it's probably not useful, otherwise can really help for us to review quicker and for devs to visualize the steps to reproduce the bugs we submit to them.

AC1: modify is_shown methods where it makes sense in yast group to take screenshots of actions being done

It was decided to have a openQA setting to be able to change the behavior from enabled default to deactivated.

Actions #1

Updated by JRivrain about 2 years ago

  • Tags set to qe-yast-refinement
Actions #2

Updated by JERiveraMoya about 2 years ago

Advantages:
You could see what some tests do without reading the code or activate the video.

Disadvantages that I see from doing this:

  • Take screenshot in control/page layer (instead of in test layer) could produce some aberrations if we don't ensure that is not applied inside wait_until, otherwise it could create a huge amount of screenshots.
  • Mix libyui with testapi (invitation to other stockholders to mess up)
  • Update old code (we should freeze old code, even if the change is harmless) as a good practice.
  • Screenshots are not needed when the test is not failing and we save resources.

IMO where we need screenshots is due the lack of granularity, so it means that is something done in the early stage of use of libyui REST API in our team. Therefore it means that we will get something really random, sometimes many screenshot for some action sometimes none, as we were not so strict at that point with having a method to check that the page is loaded.
I'd rather go with refactors "some" of them by code (of course this is more expensive), then we guarantee that you see what you want to see.

Nevertheless I don't strongly oppose to the idea, want to hear other opinions.
Otherwise if we still want to proceed this ticket would have a better scope if you could identify the places where to apply this.

Actions #3

Updated by JERiveraMoya about 2 years ago

  • Target version set to Current
Actions #4

Updated by JRivrain about 2 years ago

  • Description updated (diff)

JERiveraMoya wrote:

Advantages:
You could see what some tests do without reading the code or activate the video.

Disadvantages that I see from doing this:

  • Take screenshot in control/page layer (instead of in test layer) could produce some aberrations if we don't ensure that is not applied inside wait_until, otherwise it could create a huge amount of screenshots.
  • Mix libyui with testapi (invitation to other stockholders to mess up)
  • Update old code (we should freeze old code, even if the change is harmless) as a good practice.
  • Screenshots are not needed when the test is not failing and we save resources.

IMO where we need screenshots is due the lack of granularity, so it means that is something done in the early stage of use of libyui REST API in our team. Therefore it means that we will get something really random, sometimes many screenshot for some action sometimes none, as we were not so strict at that point with having a method to check that the page is loaded.
I'd rather go with refactors "some" of them by code (of course this is more expensive), then we guarantee that you see what you want to see.

Nevertheless I don't strongly oppose to the idea, want to hear other opinions.
Otherwise if we still want to proceed this ticket would have a better scope if you could identify the places where to apply this.

We already had some pretty long talk about that a while ago, so I did not think necessary to remind it here, but some time passed since then. Updating description with initial motivation from https://github.com/os-autoinst/os-autoinst-distri-opensuse/pull/14051. Regarding the possible abherrations, I believe we adressed it in that PR too.

Actions #5

Updated by JRivrain about 2 years ago

  • Subject changed from Modify is_shown everywhere it is possible to Modify is_shown everywhere it makes sense.
  • Description updated (diff)
Actions #6

Updated by JERiveraMoya about 2 years ago

The example that you provide is the only one I remember that trigger you to do it, when it could just have been split in two screens. I also don't like this approach with high level method, but it is how it is now.
The aberration can still be produced if we do some wrap is_shown with wait_until.
I still would like to discuss more good example of use in the refinement, otherwise for someone picking this ticket what would mean "where makes sense", make sense in old code running in maintenance?, where there is a wrap with wait_until?, etc. I guess we should search for those green tests where we don't have enough information, if you could provide some example would help. Navigate key arrow right to reach all the screenshots when with 1 or 2 is enough is also an advantage of not doing this. However let's vote it in refinement :)

Actions #7

Updated by JERiveraMoya about 2 years ago

  • Tags deleted (qe-yast-refinement)
  • Description updated (diff)
  • Status changed from New to Workable
Actions #8

Updated by JERiveraMoya over 1 year ago

  • Status changed from Workable to New
  • Priority changed from Normal to Low
  • Target version deleted (Current)
Actions #9

Updated by JERiveraMoya 3 months ago

  • Status changed from New to Rejected

Backlog clean-up.

Actions

Also available in: Atom PDF