Skip to content

[v8] Console logs and cucumber hooks support #11098

Merged
christian-bromann merged 21 commits intowebdriverio:mainfrom
sriteja777:OB-1787_console_logs
Sep 8, 2023
Merged

[v8] Console logs and cucumber hooks support #11098
christian-bromann merged 21 commits intowebdriverio:mainfrom
sriteja777:OB-1787_console_logs

Conversation

@sriteja777
Copy link
Contributor

@sriteja777 sriteja777 commented Sep 5, 2023

Proposed changes

  1. Add console logs support for observability
  2. Add cucumber hooks support for observability

Related v7 PR: #10961

Types of changes

  • Bugfix (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 not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

@sriteja777 sriteja777 changed the title Ob 1787 console logs [v8] Console logs and cucumber hooks support Sep 5, 2023
if (config.layout) {
layout = layouts.layout(config.layout.type, config.layout)
}
return BSTestOpsLog4JSAppender(layout, config.timezoneOffset)
Copy link
Member

Choose a reason for hiding this comment

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

Can we not call this "log4js"? It suggest you are using this package which you aren't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not using log4js in our code, we are just defining an appender that users can use to configure in their log4js configuration. So we don't need the log4js package for this

Comment on lines +11 to +12
export const log4jsAppender = { configure }
export const BStackTestOpsLogger = logReportingAPI
Copy link
Member

Choose a reason for hiding this comment

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

Why are these exported? Are these suppose to be used by users? If so we should provide docs for them right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes @christian-bromann these are supposed to be used by user if they want support for log4js and winston-logger. Will create a separate PR for docs, as also need to make other doc changes for observability as well

@sriteja777
Copy link
Contributor Author

@christian-bromann Have resolved comments. Please check. Also there are merge conflicts in package files due to the package added. Everytime I resolve it, the conflict appears again when a new webdriverio version releases. So will resolve it later when the PR is ready to merge. Or if you have any ideas to avoid conflicts altogether, lmk

@christian-bromann
Copy link
Member

@sriteja777 can you update the branch and resolve the conflicts?

@sriteja777
Copy link
Contributor Author

@christian-bromann as I said in this comment, the merge conflicts are appearing every time a new webdriverio version is released due to change in package files. Everytime I resolve it, the conflict appears again when a new webdriverio version releases.

So if changes look good to you, I can resolve the conflicts so that you can merge

@sriteja777
Copy link
Contributor Author

@christian-bromann Have made the requested changes. Please re-review

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Note: I can functionally test these features. I mentioned it before that it would be best to move this service into the BrowserStack org

@christian-bromann
Copy link
Member

Once rebased I am happy to merge.

# Conflicts:
#	package-lock.json
#	packages/wdio-browserstack-service/package.json
@sriteja777
Copy link
Contributor Author

@christian-bromann Have resolved conflicts. Make sure you merge this PR before merging other PRs or release, as it will create the conflicts again.

@christian-bromann christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Sep 8, 2023
@christian-bromann christian-bromann merged commit 5bba308 into webdriverio:main Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Polish 💅 PRs that contain improvements on existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants