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

[py] Implement High Level Logging API with BiDi #14107

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

p0deje
Copy link
Member

@p0deje p0deje commented Jun 7, 2024

Partial Python implementation of #13992, closely mimics Ruby part from #14073

Copy link
Member

@titusfortner titusfortner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for tests, but won't be good enough for our documentation:

add_console_message_handler(log_entries.append)

We should add a test method (I can do it in Ruby) to test filtering, where we only append errors and then click both consoleLog and consoleError buttons. That way we can show how to use lambdas and/or callbacks (and we can just copy those to the docs)

py/conftest.py Outdated Show resolved Hide resolved
@titusfortner
Copy link
Member

Yeah, this isn't really testing the code, just showing how to use it, so maybe we don't need it in specs, just in our docs. What does this look like in the python?

        driver.navigate.to url_for('bidi/logEntryAdded.html')
        log_entries = []
        driver.script.add_console_message_handler { |log| log_entries << log  if log.level == 'error'}
        driver.find_element(id: 'consoleLog').click
        driver.find_element(id: 'consoleError').click
        wait.until { log_entries.any? }
        expect(log_entries[0].level).to eq('error')

@p0deje
Copy link
Member Author

p0deje commented Jun 8, 2024

Yeah, this isn't really testing the code, just showing how to use it, so maybe we don't need it in specs, just in our docs. What does this look like in the python?

        driver.navigate.to url_for('bidi/logEntryAdded.html')
        log_entries = []
        driver.script.add_console_message_handler { |log| log_entries << log  if log.level == 'error'}
        driver.find_element(id: 'consoleLog').click
        driver.find_element(id: 'consoleError').click
        wait.until { log_entries.any? }
        expect(log_entries[0].level).to eq('error')

The equivalent would one of:

# simple lambda
errors = []
driver.script.add_console_message_handler(lambda log_entry: if log.level == 'error': errors.append(log_entry) }
# function
self.errors = []

def collect_error(log_entry)
    if log_entry.level == 'error':
        self.errors.append(log_entry)

driver.script.add_console_message_handler(collect_error}
# aggregator object
class ErrorsHandler:
    def __init__():
      self.errors = []

   def collect_error(log_entry)
      if log_entry.level == 'error':
          self.errors.append(log_entry)

errors_handler = ErrorsHandler()
driver.script.add_console_message_handler(errors_handler.collect_error}

@p0deje p0deje force-pushed the py-bidi-script branch 2 times, most recently from eadb710 to 4676beb Compare June 8, 2024 19:55
@p0deje p0deje requested a review from titusfortner June 8, 2024 19:57
Copy link
Member

@AutomatedTester AutomatedTester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@titusfortner
Copy link
Member

titusfortner commented Jun 13, 2024

@p0deje I didn't realize you added the options in here. I implemented enable_bidi() and web_socket_url parameters in trunk. You want me to rebase this?

Copy link
Contributor

codiumai-pr-agent-pro bot commented Jun 16, 2024

CI Failure Feedback 🧐

(Checks updated until commit 16e2e76)

Action: Python / Lint

Failed stage: Test with tox [❌]

Failure summary:

The action failed because the code formatting check detected that the file conftest.py is not
properly formatted according to the black code style.

  • 1 file would be reformatted: This indicates that conftest.py does not comply with the required code
    style.
  • The linting process exited with code 1, indicating a failure.

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    223:  +
    224:  request.addfinalizer(fin)
    225:  if request.node.get_closest_marker("no_driver_after_test"):
    226:  driver_instance = None
    227:  would reformat conftest.py
    228:  Oh no! 💥 💔 💥
    229:  1 file would be reformatted, 219 files would be left unchanged.
    230:  linting-ci: exit 1 (1.83 seconds) /home/runner/work/selenium/selenium/py> black --check --diff selenium/ test/ conftest.py -l 120 pid=1949
    231:  linting-ci: FAIL code 1 (5.66=setup[3.37]+cmd[0.46,1.83] seconds)
    232:  evaluation failed :( (5.82 seconds)
    233:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    Copy link

    codecov bot commented Jun 16, 2024

    Codecov Report

    Attention: Patch coverage is 18.89764% with 103 lines in your changes missing coverage. Please review.

    Project coverage is 56.98%. Comparing base (5c5487f) to head (d07cbe3).
    Report is 3 commits behind head on trunk.

    Current head d07cbe3 differs from pull request most recent head 43c5fc9

    Please upload reports for the commit 43c5fc9 to get more accurate results.

    Files Patch % Lines
    py/selenium/webdriver/common/bidi/script.py 28.98% 45 Missing and 4 partials ⚠️
    .../selenium/webdriver/remote/websocket_connection.py 7.69% 24 Missing ⚠️
    py/selenium/webdriver/common/bidi/session.py 0.00% 18 Missing ⚠️
    py/selenium/webdriver/remote/webdriver.py 15.38% 11 Missing ⚠️
    py/selenium/webdriver/common/options.py 0.00% 1 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #14107      +/-   ##
    ==========================================
    - Coverage   57.76%   56.98%   -0.79%     
    ==========================================
      Files          87       89       +2     
      Lines        5413     5528     +115     
      Branches      228      232       +4     
    ==========================================
    + Hits         3127     3150      +23     
    - Misses       2058     2146      +88     
    - Partials      228      232       +4     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @p0deje p0deje force-pushed the py-bidi-script branch 2 times, most recently from eb27327 to 16e2e76 Compare June 18, 2024 20:30
    The commit also generates Bazel test targets for BiDi-backed implementation
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    C-devtools BiDi or Chrome DevTools related issues C-py enhancement
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants