Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider a way to customize "locator description" #439

Open
yashaka opened this issue Sep 13, 2022 · 8 comments
Open

consider a way to customize "locator description" #439

yashaka opened this issue Sep 13, 2022 · 8 comments

Comments

@yashaka
Copy link
Owner

yashaka commented Sep 13, 2022

some options to implement:

1

browser.element('#abracadabra', as_'Create Document')

2

browser.element('#abracadabra').as_('Create Document')

I would say this pretty awkward not KISS way to document elements...
(see explanations in ru, starting from https://t.me/selene_py_ru/6721)

But, such feature can be made possible in context of more important #438, and once implemented could allow automatic customization of description via something like:

@with_described_locators
class MyPage:
    create_document = browser.element('#abracadabra')
    

where the with_described_locators decorator will automatically check all class (or object) fields and add custom locator descriptions to them by calling "as_(converted_human_readable_field_name)" on each field that is object of Element class

@SanKolts
Copy link

We desperately need this feature to improve allure reports in our project. I want to try to implement it.
How do you think, is it possible to implement this feature without #438 ?

@yashaka
Copy link
Owner Author

yashaka commented Feb 20, 2023

@SanKolts, you can definitely try to implement it on your side, by extending Element class or simply mankey patching the Selene's Element class. You can do it on your own and wait for "official release of this feature". Once released you will do some changes on your side to adapt to the "official version" and that's it. There should not be much changes.

Probably I would choose the "monkey patching" way if I were you. Just add the as_ method to the Element and Collection classes, that stores the description into some special private variable, something like ___description (use three undesrcores and it will never interfere with something on Selene's side). Then use this ___description to log element in the allure report.

Share your results here... How did it go, what were the pitfalls.

In meantime I will think on how best to implement this feature on Selene's side, taking into account the #438... For now I can hardly predict how these issues may be connected...

@SanKolts
Copy link

SanKolts commented Feb 24, 2023

I did the monkey-patching as a proof of concept.

Test data

Let's start with some BDD data to understand what we want to achieve. I will not add cases for Collection because it doesn't change the main idea.

  1. browser.element('.login-form').as_('auth_form').element('[label="Email"]').as_('email_field').click()
    should render a step browser.auth_form.email_field: click
  2. browser.element('.login-form').element('[label="Email"]').click()
    should render a step browser.element('.login-form').element('[label="Email"]'): click
  3. browser.element('.login-form').element('[label="Email"]').as_('email_field').click()
    should render a step browser.element('.login-form').email_field: click
  4. browser.element('.login-form').as_('auth_form').element('[label="Email"]').click()
    should render a step browser.auth_form.element('[label="Email"]'): click

As you can see, we need to preserve an element name chain plus use a locator description if as_() is not provided. To do so, we need:

  1. an element description (self.___description) in my code)
  2. an element pre-path string (self.___path_description in my code)
  3. a locator description(Locator.last_locator() in my code)
  4. a locator path string (str(self.locator) in Selene's Element)

It is quite challenging to implement (especially via monkey patching). But after it is implemented, adding the feature of auto-adding element name's become very easy. You just need to add:

class BasePage
    def __setattr__(self, key, value):
        if isinstance(value, WaitingEntity):
            value.as_(key)
        super().__setattr__(key, value)

and after that, every inherited page will add a corresponding attribute name as a description for Selene's Elements.

Here are the real examples from our project.

Before the changes:
Screenshot 2023-02-24 at 17 13 54

after the changes:
Screenshot 2023-02-24 at 17 13 40

Here is my dirty monkey patching implementation to play with:

     # we need the last part of the locator for use as a name in case a description wasn't provided
    @monkey.patch_method_in(Locator)
    def last_locator(self):
        result = re.search(r'''(element|all)\(\('[^()]*', '[^']*'\)\)$''', self._description)
        return result.group()

    WaitingEntity.___description = None
    WaitingEntity.___path_description = None

    @monkey.patch_method_in(WaitingEntity)
    def as_(self, name: str):
        self.___description = name
        return self

    @monkey.patch_method_in(WaitingEntity)
    def ___locator_description(self):
        locator_description = str(self._locator) if hasattr(self, '_locator') else ''
        return locator_description

    @monkey.patch_method_in(Collection)
    def __str__(self):
        if self.___path_description:
            result = self.___path_description + '.' + (self.___description or str(self._locator.last_locator()))
        else:
            result = str(self._locator)
        return result

    @monkey.patch_method_in(Element)
    def __str__(self):
        if self.___path_description:
            result = self.___path_description + '.' + (self.___description or str(self._locator.last_locator()))
        else:
            result = str(self._locator)
        return result

    @monkey.patch_method_in(Browser)
    def element(self, css_or_xpath_or_by: Union[str, tuple]) -> Element:
        by = to_by(css_or_xpath_or_by)

        patched_element = Element(
            Locator(f'{self.___locator_description()}.element({by})', lambda: self.driver.find_element(*by)),
            self.config,
        )
        patched_element.___path_description = str(self)
        return patched_element

    @monkey.patch_method_in(Browser)
    def all(self, css_or_xpath_or_by: Union[str, tuple]) -> Collection:
        by = to_by(css_or_xpath_or_by)
        patched_collection = Collection(
            Locator(f'{self.___locator_description()}.all({by})', lambda: self.driver.find_elements(*by)),
            self.config,
        )
        patched_collection.___path_description = str(self)
        return patched_collection

    @monkey.patch_method_in(Element)
    def element(self, css_or_xpath_or_by: Union[str, tuple]) -> Element:
        by = to_by(css_or_xpath_or_by)

        patched_element = Element(
            Locator(f'{self.___locator_description()}.element({by})', lambda: self().find_element(*by)),
            self.config,
        )
        patched_element.___path_description = str(self)
        return patched_element

    @monkey.patch_method_in(Element)
    def all(self, css_or_xpath_or_by: Union[str, tuple]) -> Collection:
        by = to_by(css_or_xpath_or_by)
        patched_collection = Collection(
            Locator(f'{self.___locator_description()}.all({by})', lambda: self().find_elements(*by)),
            self.config,
        )
        patched_collection.___path_description = str(self)
        return patched_collection

    @monkey.patch_method_in(Collection)
    def all(self, selector: Union[str, tuple]) -> Collection:
        by = to_by(selector)

        patched_collection = Collection(
            Locator(
                f'{self.___locator_description()}.all({by})',
                lambda: flatten(
                    [webelement.find_elements(*by) for webelement in self()]
                ),
            ),
            self.config,
        )
        patched_collection.___path_description = str(self)
        return patched_collection

I'm not sure that I want to add it to my project as it is :)

I have some ideas on how to implement it properly in Selene.

@yashaka
Copy link
Owner Author

yashaka commented Feb 24, 2023

I'm not sure that I want to add it to my project as it is :)

Why not? Looks like a fast and pretty nice solution:) until we find the best way to implement it in Selene:)

@yashaka
Copy link
Owner Author

yashaka commented Feb 24, 2023

@SanKolts

I liked your approach on «getting things done» in the last example of yours.

Yet for Selene... I am not sure that this approach:

browser.element('.login-form').as_('auth_form').element('[label="Email"]').as_('email_field').click()
should render a step browser.auth_form.email_field: click

is the one to follow...

It is actually... Pretty «opinionated», a bit not consistent with a «general OOP-like way»...

Look at this:

element: Element = None

element = browser.element('#foo')

print(element) # => browser.element('#foo') 

described = element.as_('foo')

print(described) # => ?

In Selene we try to follow the following API Design strategy: «while documentation is very welcome, the majority of users should be able to guess what some command does just by looking at its signature and usage»...

Given we have not read any docs... What can we guess on «actual result» of the latest code?

I would guess that it would print -> "foo"

Because we kind of say, log element now as "foo" instead of default.

This is actually the simplest guess we can do... And the simplest (i.e. the KISS) thing to implement. What as_ should do? just overwrite the describtion by default. Overwriting just «some part depending on context» would be definitely not KISS, harder to guess...

Yet, actually, even your implementation would print browser.foo...

Now look at this... we have shown to the user that element.as('foo') would print browser.foo...

Then what would a user guess in the following example:

element: Element = None

element = browser.element('#foo')

print(element) # => browser.element('#foo') 

described = element.as_('foo')  # <- NOTICE THAT HERE

print(described) # => browser.foo

element = element.element('#inner-foo')

described = element.as('inner-foo') # <- AND HERE THE USAGE LOOKS SAME

print(described) # => ?

Now what?

Taking into account, that as_ is called yet on an element object... and we already know that as_ overwrites the default locator-like description... I would guess that in the last example, we still do the same - just ovewrite it and have something like:

print(described) # => browser.inner-foo

Why not? :) Pretty KISS and straightforward:)

Probably even more KISS and obvious in context of «guessing» would be something like:

element: Element = None

element = browser.element('#foo')

print(element) # => browser.element('#foo') 

described = element.as_('foo')  # <- NOTICE THAT HERE

print(described) # => foo

element = element.element('#inner-foo')

described = element.as('inner-foo') # <- AND HERE THE USAGE LOOKS SAME

print(described) # => inner-foo

This would be pretty standard in context of common OOP. When we have an element - we have an element:) It remembers what was given to him on creation. It should not remember the path how it was created. It just does not matter. When a new child had been born, it should not get sins of its parents unless they explicitely taught the child during education:)

Look also at this:

@with_described_locators
class MyPage:
    def __init__(self):
        self.form = browser.element('#form')
        self.submit = self.form.element('#submit')

If implemented as you did in your example (let's call it the DSL-like implementation), it would give an exellent report on usage, something like:

* browser.form: click [MyPage]
* browser.form.submit: click [MyPage]

But should we even bother with much harder implementation and being «harder guessable without docs» if we still can achieve same with «more KISS and guessable version»:

@with_described_locators
class MyPage:
    def __init__(self):
        self.form = browser.element('#form')
        self.form_submit = self.form.element('#submit')

and in the report:

* form: click [MyPage]
* form-submit: click [MyPage]

In most cases, following the Self-Documented Code principle, we nevertheless should give to our object fields – the descritive self-explanatory names. I.e. if have written the proper «clean code», then the report would be completely readable even with more KISS and straightforward «standard OOP-like» implementation:)

And... Yes, I believe that «the default as_ behavior in Selene» should be KISS, but... Yet it would be good to implement it in the way, that it can be easyly configurable, without monkey-patching, to what user prefers. And the end user of Selene prefers more DSL-like behavior as you proposed above, then it should be pretty straightforward in Selene to do :).

Now comes the question, how to implement it so everybody would be happy:)

@SanKolts
Copy link

SanKolts commented Feb 27, 2023

Why not? Looks like a fast and pretty nice solution:) until we find the best way to implement it in Selene:)

I'm afraid it break in the next Selene update due to many changes.

Yes, you spotted here the main idea of what I thought while monkey patching:

element: Element = None

element = browser.element('#foo')

print(element) # => browser.element('#foo') 

described = element.as_('foo')  # <- NOTICE THAT HERE

print(described) # => browser.foo

element = element.element('#inner-foo')

described = element.as('inner-foo') # <- AND HERE THE USAGE LOOKS SAME

print(described) # => ?

In my project, we implemented components-based architecture to DRY because it is SPA. So

class MyPage:
    def __init__(self):
        self.form = browser.element('#form')
        self.form_submit = self.form.element('#submit')

is not an option, because we could have nested objects. For example, we have:

class Modal(BaseElement):

    def __init__(self):
        self.close_button: Element = self._container.element('header button')
        self.header: Element = self._container.element(self.header_locator)

class SomePage:

    def __init__(self):
        self.some_modal = Modal()

If I use it without a path, my code is very redundant:
page.some_modal.some_modal_close_button.click()

Even worse, it becomes impossible to log inherited attributes clearly: If I have 10 modal classes and each of them has the same close_button, all of them will be just close_button.

On the other hand, if you store the path and the name of the current element, it is possible to print only the name of the element by monkey patching of __str__ method.
Also, I think it will be good to leave an option to set the element name later in the code and apply it to any descendants. It will help to use the components approach.

To implements this, Selene needs something like:

class ChainableDescription:
    def __init__(self, name, prev_description: 'ChainableDescription' = None):
        self.prev_description = prev_description
        self.name = name

    def resolve(self) -> Iterable:
        name_chain = []
        if self.prev_description:
            name_chain.append(self.prev_description.resolve())
        name_chain.append(self.name)
        return name_chain

@yashaka
Copy link
Owner Author

yashaka commented Feb 27, 2023

@SanKolts

If I use it without a path, my code is very redundant:
page.some_modal.some_modal_close_button.click()

You code is rendundant not because of "selene might lack remembered path" but because you made a design with tautology, i.e. it was your choice:)

If you notice, my example does not have tautology in naming:

@with_described_locators
class MyPage:
    def __init__(self):
        self.form = browser.element('#form')
        self.form_submit = self.form.element('#submit')

And still, in the report...

* form: click [MyPage]
* form-submit: click [MyPage]

– I can clearly see to which component (in this case its name is MyPage) my element belongs to.

Same can be implemented in a more complicated example, something like this:

@with_described_locators
class Modal:

    def __init__(self, container):
        self._container = container
        self.close_button = self._container.element('#close') 
        """
        # on modal.close_button.click() will report:
        # > close_button: click [Modal]
        """
        
@with_described_locators
class SomePage:

    def __init__(self):
        self.some_modal = Modal()
        """
        # on some_page.some_modal.close_button.click() will report:
        # > close_button: click [Modal][SomePage]
        # or depending on you logging implementation may also be:
        # > close_button: click [SomePage][Modal]
        # or even:
        # [SomePage][Modal] close_button: click 
        """

For all this, you don't need any complex «description creation path» implementation on Selene's side. You just properly implement your @with_described_locators decorator (that is by the way, in most cases better option than using inheritance). And you don't need to monkey patch Selene for that, so no conflicts in future. Then your monkey-patch for "custom description" will be much simpler, and once you store it in a field with some unique prefix – you will also won't have conflicts with newer Selene's releases.

You see, my main idea is to keep things simple. Selene should not have over-knowledge, should not be an opinionated framework. It should be SIMPLE and POWERFUL through customization. Simple things make life easier ;) Check "Simple Made Easy" - Rich Hickey (2011)

And recall the Single Responsibility Principle. Customizing element description - is one responsibility. Remembering the "elements creation path" - is another responsibility. Ask yourself – where do you need this second responsibility? - In the architecture of nested components? - Yes. Then get your main tool for this architecture (probably a base component class at your current context) and implement there the corresponding logging of "remembered path in a chain of nested components". Selene is about lazy elements, not about how to use them, in what chain of nested components. We should not mixture responsibilities here. And should not introduce the corresponding tight coupling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants