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

Discussion: Error messages #1933

Open
Lakitna opened this issue Feb 28, 2025 · 2 comments
Open

Discussion: Error messages #1933

Lakitna opened this issue Feb 28, 2025 · 2 comments

Comments

@Lakitna
Copy link

Lakitna commented Feb 28, 2025

I've been using SeleniumLibrary for a while now. It's been great. I have come across issues and headaches, but almost all of them are caused by Selenium itself :)

Context

We're using SeleniumLibrary within a significantly sized project with 10+ testers. Some of these testers are not well-versed in code or technical things. Because of this situation, I find myself wrapping most SeleniumLibrary keywords for two reasons: adding retrying behavior and enhancing the error messages. I want to focus on the second reason for this issue.

Our error messages

To me, the error messages are a form of documentation. Documentation that pops up the exact moment you need to know about something. Because of this idea, I write error messages like this:

Element is not visible | Page: 'https://example.com/page' | Element 'div.example.locator' not visible after 5 seconds | Screenshot: 0-FAIL-meaningful-name.png

This error message has the following parts:

  1. Element is not visible

    Short description without details. This is useful for quickly scanning multiple fails, especially in our pipeline test report. In the pipeline report, we only have the test name and the fail message. The short message allows you to decide if you want more details. If you do, you keep reading.

  2. Page: 'https://example.com/page'

    On what page did the error occur? This is useful for debugging and recognizing known issues. I think known issues should not exist, but reality teaches us that they happen. Being able to recognize them is never a bad thing.

  3. Element 'div.example.locator' not visible after 5 seconds

    The actual details of what went wrong. This part can consist of multiple parts, depending on the keyword. The idea is to give as many details as possible. These details are used to figure out what exactly happened. Other examples from our codebase include:

    Simple one:
    Could not scroll element 'div.example.locator' into view

    Multiple parts:
    Element 'div.example.locator' contains unexpected text after 5 seconds | 'Foo' (expected) in 'Who is this Foo fella?' (actual)

    Multiple parts with lots of details:
    Element 'div.example.locator img' attribute 'alt' does not contain 'foo' after 5 seconds | 'Amazing image alt tag contents' does not contain 'foo'

  4. Screenshot: 0-FAIL-meaningful-name.png

    The file name of the screenshot. This allows you to easily reference a screenshot if it was made. If no screenshot is made, this part will be Screenshot: [None].

Examples

Same example as above

Element is not visible | Page: 'https://example.com/page' | Element 'div.example.locator' not visible after 5 seconds | Screenshot: 0-FAIL-meaningful-name.png

Lots of details

Element attribute value does not contain expected value | Page: 'https://example.com/page' | Element 'div.example.locator' attribute 'class' does not contain 'foo' after 5 seconds | 'lorum ipsum' does not contain 'foo' | Screenshot: 0-FAIL-meaningful-name.png

Note that the 3rd part is the exact error from Wait until Location contains

Unexpected location | Page: 'https://example.com/page' | Location did not contain 'https://example.com/page' in 5 seconds. | Screenshot: 0-FAIL-meaningful-name.png

Note that the 3rd part is the exact error from Wait Until Element Is Enabled

Element is not enabled | Page: 'https://example.com/page' | Element 'div.example.locator' not enabled in 5 seconds | Screenshot: 0-FAIL-meaningful-name.png

Discussion topic

I'm a fan of solving something like this in a great way, only once, in the right place. I think the right place for these error messages is in SeleniumLibrary. I also think the existing error messages are pretty good but can be improved.

What do you think about adding (some of) these ideas to SeleniumLibrary?

Please note that I'm willing to help implement these changes.

@emanlove
Copy link
Member

Overall I think this is a very good idea. In my mind one of the roles libraries play is to simplify the process for using a underlying tool, like Selenium. So something like translating maybe obscure error messages makes sense and really fit into the larger purpose or functionality I think our libraries should perform.

I would be curious then to look at the next layer down which is around the technical specifics looking at ..

  • Is it a match the message or an exception?
  • Noting that selenium randomly changes its error message if match by message what would be the way to resolve this?
  • Are the error messages that don't get translated? Or are all covered? And how does one guarantee that?
  • How do the log level come into play here?
  • ??? maybe some more here

I would also like to see a side by side comparison of what we currently do and what the proposal would be. But overall I think this is a good idea which we should discuss more.

@Lakitna
Copy link
Author

Lakitna commented Mar 2, 2025

Awesome that you like the (general) idea :)

For the technical parts, there are some things I noticed from doing this in our code base.

Only the message matters

... to users. I think that there are 2 parts to this:

Almost all uses of SeleniumLibrary will be in Robot rather than Python. For example, I write most keywords in Robot because very few people in the team can write Python. In Robot, an error is always a string so for all intents and purposes error object == error message.

When you use SeleniumLibrary in Python, the error object can matter. I think that an error object that allows easy error message construction would be really nice. For example, I currently use this error constructor:

Disclaimer: This code is 'good enough' for me, but probably not for here

class CompanyNameRobotFailure(Exception):
    short_message: str = None
    url: str = None
    long_message: str = None
    error: str = None

    def __init__(
        self, short_message=None, long_message=None, page_url=None, error_message=None
    ) -> None:
        if isinstance(error_message, CompanyNameRobotFailure):
            self.short_message = error_message.short_message
            self.long_message = error_message.long_message
            self.url = error_message.url
            self.error = error_message.error
        else:
            self.short_message = short_message
            self.long_message = long_message

            if page_url:
                page_url = f"Page: '{page_url}'"
            self.url = page_url

            if isinstance(error_message, Exception):
                error_message = str(error_message)
            self.error = error_message

        parts = [
            self.short_message,
            self.url,
            self.long_message,
            self.error,
        ]
        parts = [p for p in parts if p is not None and p != ""]

        super().__init__(" | ".join(parts))
raise CompanyNameRobotFailure(
    short_message="Element is not visible",
    long_message=f"Element '{locator}' not visible after {timeout_str}",
    page_url=self._get_driver().current_url,
    error_message=e,
)

I imagine a SeleniumLibraryError constructor that allows for easy message building. The stringified version of this object creates a standard message structure as discussed earlier.

Build the error message at the end

Regardless of the keyword, you probably have all the information needed for a complete error message in the keyword function. There are some exceptions, I'll come back to those.

With this idea, my keywords look something like this:

*** Keywords ***
Our Custom Get Element attribute
    [Arguments]    ${element}    ${attribute}    ${timeout}=${TIMEOUT_DEFAULT}
    Our Custom wait on element    ${element}    timeout=${timeout}

    TRY
        ${value} =    Wait Until Keyword Succeeds    ${timeout}    ${TIMEOUT_DEFAULT_RETRY_INTERVAL}
        ...  GEN-private GUI Get Element attribute    ${element}    ${attribute}
        RETURN    ${value}
    EXCEPT    * NO_ATTRIBUTE    type=GLOB
        ${location} =    SeleniumLibrary.Get Location
        ${timeoutString} =    Convert Time    ${timeout}    verbose
        Our Custom Fail
        ...  message=Element has missing attribute | Page: '${location}' | Element '${element}' has no attribute '${attribute}' after ${timeoutString}
        ...  screenshotName=Get Element attribute
        ...  screenshotHighlight=${element}
    END

Private Our Custom Get Element attribute
    [Documentation]    Retryable part of GEN Wait and Get Element attribute
    [Tags]    robot:private    robot:flatten
    [Arguments]    ${element}    ${attribute}
    ${actualValue} =    SeleniumLibrary.Get Dom Attribute    ${element}    ${attribute}

    IF    $actualValue == $None
        Fail    NO_ATTRIBUTE
    END

    RETURN    ${actualValue}

There is a lot of code here dedicated to making a meaningul error message. What I want to highlight here is that the error message is constructed in the keyword itself. The actual logic of what we're waiting on is separated to the secondary (private) keyword.

If you look at the code, you may spot some features we're ignoring for this discussion. If you're interested in those, we can create a spin-off discussion ;)

By building the error messages in the keyword itself, you can easily see which keywords use an extended error message without digging into the code.

This also allows you to overwrite any error messages created deeper in the code, if you whish. However, this is a double edged sword. Because of this, I sometimes don't even touch the detailed error message. I may do something like:

*** Keywords ***
Our Company Location Should Contain
    [Arguments]    ${partialUrl}    ${timeout}=${TIMEOUT_DEFAULT}
    TRY
        SeleniumLibrary.Wait until Location contains
        ...  expected=${partialUrl}
        ...  timeout=${timeout}
    EXCEPT    AS    ${e}
        GEN GUI Fail
        ...  message=Unexpected location | ${e}
        ...  screenshotName=Location Should Contain
    END

Here, I simply pass along ${e} as the detailed error message.

Your questions

I already attempted to cover your questions above, but for clarity sake:

Is it a match the message or an exception?

It'll probably be a whole new exception. The messages inside that exception can come from multiple places.

Noting that selenium randomly changes its error message if match by message what would be the way to resolve this?

I would replace most/all Selenium error messages. I would do this by writing the error messages in the top-level keyword functions.

Are the error messages that don't get translated? Or are all covered? And how does one guarantee that?

All/most are translated. This is guaranteed by doing it in the top-level keyword functions and by writing tests that assert the expected error message.

How do the log level come into play here?

I think it does not. This should be about the contents of error messages. I don't think it's worth changing the contents of error messages based on the log level.

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

No branches or pull requests

2 participants