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

[rb]: Standardise driver logging output #9850

Merged
merged 2 commits into from
Sep 27, 2021

Conversation

luke-hill
Copy link
Contributor

Ensure all log formats (Including browser ones), are in a consistent format

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Alter logging to conform to other standard log formats

Motivation and Context

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.

@luke-hill
Copy link
Contributor Author

luke-hill commented Sep 20, 2021

Failure seems unrelated

 1) Selenium::WebDriver::DevTools network interception changes requests
     Failure/Error: expect(driver.find_elements(tag_name: 'img').map(&:size).uniq).to eq([Dimension.new(640, 480)])
     
       expected: [#<struct Selenium::WebDriver::Dimension width=640, height=480>]
            got: [#<struct Selenium::WebDriver::Dimension width=0, height=0>, #<struct Selenium::WebDriver::Dimension width=640, height=480>]
     
       (compared using ==)

Not really sure what happened here, guessing there is a secondary blank 0x0 image somewhere on the page it's finding?

@titusfortner
Copy link
Member

This isn't a backward compatible change and the code isn't marked as @api private, even if maybe it should be. What are you trying to do and how does this change make it better?

Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

I don't quite follow what do we standard against. Do you want the browser logs to look like Ruby's Logger class?

@@ -22,22 +22,22 @@ module WebDriver
class LogEntry
attr_reader :level, :timestamp, :message

def initialize(level, timestamp, message)
@level = level
def initialize(timestamp, level, message)
Copy link
Member

Choose a reason for hiding this comment

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

Is there any value to change the order here? Even though nobody should be initializing this class directly, I don't believe it's worth breaking backwards compatibility without a good reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll undo this change, but it was only used in the entire codebase in one location, which was api private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry yes. The other logger formats in a different way, and the timestamp is always first in most loggers. Here it wasn't.

@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@luke-hill
Copy link
Contributor Author

Is this RtM now @p0deje (The ruby failure seems to find a 0x0 reference).

@p0deje
Copy link
Member

p0deje commented Sep 22, 2021

@luke-hill Could you please reply to #9850 (review)?

@p0deje p0deje merged commit 016eded into SeleniumHQ:trunk Sep 27, 2021
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

Successfully merging this pull request may close these issues.

3 participants