Skip to content

Conversation

@amaanbs
Copy link
Owner

@amaanbs amaanbs commented Oct 24, 2024

Proposed changes

Types of changes

  • Polish (an improvement for an existing feature)
  • 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 (improvements to the project's docs)
  • Specification changes (updates to WebDriver command specifications)
  • Internal updates (everything related to internal scripts, governance documentation and CI files)

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 the necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Backport Request

//: # (The current main branch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against the v8 branch.)

  • This change is solely for v9 and doesn't need to be back-ported
  • Back-ported PR at #XXXXX

Further comments

Reviewers: @webdriverio/project-committers

@sauravdas1997
Copy link

LGTM, lets, get another review

import type { Options } from '@wdio/types'

import { BROWSERSTACK_TESTHUB_UUID } from '../constants.js'
const logDir = 'logs'

Choose a reason for hiding this comment

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

Please add a line after import statements

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done

}
if (percySnapshot && percySnapshot.percyScreenshot) {
screenshotHandler = percySnapshot.percyScreenshot
screenshotHandler = (browser: WebdriverIO.Browser | WebdriverIO.MultiRemoteBrowser | string, screenshotName: any, options?: any) => {

Choose a reason for hiding this comment

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

Can we give types to screenshotName and options?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Percy screenshot method accepts arguments with method overloading. Arguments accepted are either (browser, name, options) or (name, options) where options can be in any format. Hence I've kept the parameter types to any.

Choose a reason for hiding this comment

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

screenshotName should be string only no?

}
if (percyAppScreenshot) {
screenshotAppHandler = percyAppScreenshot
screenshotAppHandler = (driverOrName: any, nameOrOptions?: any, options?: any) => {

Choose a reason for hiding this comment

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

here also

Copy link
Owner Author

Choose a reason for hiding this comment

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

}

if (this._options.testObservability) {
const shouldSetupPercy = this._options.percy || (isUndefined(this._options.percy) && this._options.app)

Choose a reason for hiding this comment

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

percy auto-enabled needs to be handled?

Copy link
Owner Author

Choose a reason for hiding this comment

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

The condition after || is for auto enabling percy for App sessions.

if (!passed) {
this._failReasons.push((error && error.message) || 'Unknown Error')
}
await this._accessibilityHandler?.afterTest(this._suiteTitle, test)

Choose a reason for hiding this comment

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

why changing order?

Copy link
Owner Author

Choose a reason for hiding this comment

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

So that the data being populated for saving Accessibility test results is correctly populated (ref) (specifically test uuid)

import { BStackLogger } from '../bstackLogger.js'

import { shouldProcessEventForTesthub } from '../testHub/utils.js'
class Listener {

Choose a reason for hiding this comment

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

Please add line after import

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done.


try {
const url = `${DATA_ENDPOINT}/${eventUrl}`
const url = `${ENDPOINT}/${eventUrl}`

Choose a reason for hiding this comment

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

ENDPOINT ? intended ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Changed.

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.

5 participants