abalakh / airgun

Airgun moved to https://github.com/SatelliteQE/airgun
https://github.com/SatelliteQE/airgun
1 stars 2 forks source link

Keeping the read/fill interface #11

Open mfalesni opened 6 years ago

mfalesni commented 6 years ago

I noticed that one of your widgets https://github.com/abalakh/airgun/blob/master/airgun/widgets.py#L51 does not conform to one of the concepts WT is built on.

Whatever is returned by read() must be acceptable by fill(). So essentially, x.fill(x.read()) should work at any time.

abalakh commented 6 years ago

@mfalesni your suggestion on how to handle such widgets? It looks like that:

screen shot 2018-02-08 at 3 10 55 pm

Depending on operation we may want to select something from left list and move it to the right one, or we may want to remove it from right list. That's why operation key was added for fill. But there's obviously no such thing like operation when we're reading existing elements.

mfalesni commented 6 years ago

Let it accept and return the dictionary with selected/all keys and decide based upon that? Of course you can have a special form of fill that can do a specific thing, like adding, but whatever read returns should be digestible by fill.

abalakh commented 6 years ago

Let it accept and return the dictionary with selected/all keys and decide based upon that?

Won't usability suffer from that? Right now from tests level we call it with 2 values - one is operation to perform, second is element to perform action to. Pretty clear. With 2 lists person who writes the test should know about existence of 2 lists and should decide to which list to include element - either 'available' or 'selected'. Which is not descriptive btw: pls compare {'operation': 'Remove', value: 'something'} and {'available': 'something'} (which are equal, although second option doesn't tell me that i'm removing something) Or am i missing your point?

mfalesni commented 6 years ago

The basic idea is that read tells you what is on the screen and you tell fill what should be on the screen. One of the solutions may be as used in our MultiBoxSelect which is the type of widget you have: https://github.com/ManageIQ/integration_tests/blob/master/widgetastic_manageiq/__init__.py#L147

My only concern is, that whatever read() returns must be digestible by fill(). So you can have it so if you pass a dictionary with the two lists it will make sure that the two boxes in the page have the items as described in the list (think of that scenarion like a sort of ansible for the UI :smiley:). But of course, you can make a special case eg. with the operation, it will only add, or only remove, and won't touch other selected items. Because that is an extra on fill's side, not read.

jhutar commented 6 years ago

Hmm, with assign_resource() and unassign_resource() you to not need to tweak that fill() method because you already have a way to do what you are describing, do I read the code correctly? Then fill can be something like:

def fill(self, values):
    current = self.read()
    extra = list(set(current) - set(values))
    missing = list(set(current) - set(values))
    self.unassign_resource(extra)   # or vice versa, not sure
    self.assign_resource(missing)   # or vice versa, not sure

def read(self):
    return [el.text for el in self.browser.elements(self.LIST_TO)]

Would something like this work for everybody?

mfalesni commented 6 years ago

@jhutar I think they do not want to bother with all the values and want to be able to just tell it "Hey, add this one". But you are right, this is how the fill would look for the general use case.

Btw, @abalakh & @jhutar , it is good in such case to have the logic of reading pulled into a method or a property, so read just returns that method's result or property value and fill uses the method or property to get the value. This is somewhat cosmetic change, but it makes read not appear in logs during fill.