Skip to content

[rb] move logging and rspec matchers to top namespace#12298

Closed
titusfortner wants to merge 1 commit intotrunkfrom
rb_logagain
Closed

[rb] move logging and rspec matchers to top namespace#12298
titusfortner wants to merge 1 commit intotrunkfrom
rb_logagain

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Jul 1, 2023 •
edited
Loading

Description

  • move custom logger RSpec matchers to same place as logger
  • matchers must match both ID and log level/type
  • matchers can work with logs that aren't Selenium::WebDriver.logger (requires forwarding progname)
  • allow matching multiple log ids
  • matcher log level names are past-tense

Motivation and Context

  • Would like to be able to use the matchers in other projects (Watir)
  • Previously added support for matching any log level but didn't enforce it, only the ids
  • Contemplating moving these files into a separate package for Selenium 5

Considerations

Should this file be namespaced?
Is there a better alternative to passing in a custom logger with each method?

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)

@titusfortner titusfortner requested a review from p0deje July 1, 2023 18:36
@titusfortner
Copy link
Member Author

titusfortner commented Jul 2, 2023 •
edited
Loading

Also, should we move the self.logger method out of selenium/webdriver.rb and into selenium/webdriver/common/logger.rb? If a class like Selenium::Server were to use it, it wouldn't need to pull in all of Selenium to make that work.

@titusfortner titusfortner changed the title [rb] move custom logger rspec matchers to lib [rb] move logging and rspec matchers to top namespace Jul 24, 2023
p0deje
p0deje reviewed Jul 24, 2023
View reviewed changes
rb/lib/selenium/logger/logger_matchers.rb Outdated
debugged: 'DEBUG'}.freeze

LEVELS.each_key do |level|
RSpec::Matchers.define :"have_#{level}" do |expected_ids, logger = nil|
Copy link
Member

@p0deje p0deje Jul 24, 2023

Choose a reason for hiding this comment

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

This will be loaded in runtime where RSpec might be unavailable. It would be better to keep it in rb/spec/ so it's not part of the gem itself.

@titusfortner
Copy link
Member Author

titusfortner commented Jul 24, 2023

I put this at top level in case we want to release a separate selenium-logger.gem, or, even, split this code into a separate repo/project since it isn't part of "core" Selenium. It will also make it easier to have separate loggers for devtools gem or server class or selenium-manager if we make that its own gem as well.

@p0deje
Copy link
Member

p0deje commented Jul 24, 2023

I put this at top level in case we want to release a separate selenium-logger.gem, or, even, split this code into a separate repo/project since it isn't part of "core" Selenium. It will also make it easier to have separate loggers for devtools gem or server class or selenium-manager if we make that its own gem as well.

You would then need to make sure that matchers are not loaded unless RSpec is available.

@titusfortner titusfortner marked this pull request as draft July 25, 2023 03:39
@titusfortner titusfortner force-pushed the rb_logagain branch 2 times, most recently from fd2b9e5 to d473082 Compare July 25, 2023 18:29
@titusfortner
Copy link
Member Author

titusfortner commented Jul 25, 2023

Yeah, this needed several pieces of tidying still.

  • everything namespaced Selenium::Logger
  • rescuing LoadError if RSpec not installed
  • removed hard coded reference to Selenium::WebDriver.logger, which means users have to explicitly pass in the logger being used

@titusfortner titusfortner marked this pull request as ready for review July 25, 2023 18:32
@titusfortner titusfortner mentioned this pull request Jul 27, 2023
Closed
@titusfortner titusfortner marked this pull request as draft July 27, 2023 17:51
@github-actions github-actions bot added the Stale label May 2, 2024
@github-actions github-actions bot closed this May 16, 2024
@titusfortner titusfortner added J-stale Applied to issues that become stale, and eventually closed. and removed Stale labels Mar 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@p0deje p0deje p0deje approved these changes

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

J-stale Applied to issues that become stale, and eventually closed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@titusfortner @p0deje