Skip to content

Replace calls to console.log with managed loggers#12909

Merged
diemol merged 3 commits intoSeleniumHQ:trunkfrom
danielrozenberg:js-loggers
Oct 11, 2023
Merged

Replace calls to console.log with managed loggers#12909
diemol merged 3 commits intoSeleniumHQ:trunkfrom
danielrozenberg:js-loggers

Conversation

@danielrozenberg
Copy link
Contributor

@danielrozenberg danielrozenberg commented Oct 9, 2023

Description

This PR replaces calls to console.<loglevel>() in JS code with (hopefully) appropriate Selenium-managed loggers.

This also removes hardcoded logger names in existing managed logger calls with the referenced names (e.g., replacing logging.getLogger('webdriver') with (logging.getLogger(logging.Type.DRIVER))

Motivation and Context

Fixes #12676
(and related instances of the same logging issue that don't have a reported issue on GH)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@danielrozenberg danielrozenberg marked this pull request as ready for review October 9, 2023 18:07
@danielrozenberg
Copy link
Contributor Author

danielrozenberg commented Oct 9, 2023 •
edited
Loading

Hey Selenium team! I'm pretty confident about all tests passing, but didn't want to tick that box because I wasn't able to run all of them on my workstation, only by removing edge and firefox tests from the runner. The changes are in the node directory so I don't think they should be affected

When reviewing, please pay extra attention to which loggers I used in each instance. I think my assumptions are grounded, but you never know :D

@danielrozenberg danielrozenberg mentioned this pull request Oct 9, 2023
Closed
@pujagani pujagani added the C-nodejs JavaScript Bindings label Oct 10, 2023
harsha509
harsha509 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@harsha509 harsha509 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 Oct 10, 2023

Sorry, not in a place to parse through the JS right now.

Which of the logging features in our docs - https://www.selenium.dev/documentation/webdriver/troubleshooting/logging/
does this PR address?

@danielrozenberg
Copy link
Contributor Author

danielrozenberg commented Oct 10, 2023 •
edited
Loading

This PR replaces uses of console.log (sorta JS's equivalent of printf to stdout) with Selenium's proper loggers

@diemol
Copy link
Member

diemol commented Oct 11, 2023

This is why https://www.selenium.dev/documentation/webdriver/troubleshooting/logging/ was not working in all cases for JS, we were not leveraging the logger library.

Thanks, @danielrozenberg!

@diemol diemol merged commit a717e3f into SeleniumHQ:trunk Oct 11, 2023
@titusfortner
Copy link
Member

titusfortner commented Oct 11, 2023

Thanks!

@danielrozenberg danielrozenberg deleted the js-loggers branch October 13, 2023 18:01
aguspe pushed a commit to aguspe/selenium that referenced this pull request Oct 22, 2023
@danielrozenberg @harsha509
@diemol @aguspe
Replace calls to console.log with managed loggers (SeleniumHQ#12909)
d808acc 
Co-authored-by: Sri Harsha <12621691+harsha509@users.noreply.github.com>
Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@diemol diemol diemol approved these changes

@harsha509 harsha509 harsha509 approved these changes

@AutomatedTester AutomatedTester Awaiting requested review from AutomatedTester

Assignees

No one assigned

Labels

C-nodejs JavaScript Bindings

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: unnecessary console.debug messages in seleniumManager.js

6 participants

@danielrozenberg @CLAassistant @titusfortner @diemol @harsha509 @pujagani