Skip to content

Support for Jasmine sessions for BrowserStack Test Observability (v7)#10173

Merged
christian-bromann merged 50 commits intowebdriverio:v7from
sriteja777:ob_jasmine_fix_build_v7
May 22, 2023
Merged

Support for Jasmine sessions for BrowserStack Test Observability (v7)#10173
christian-bromann merged 50 commits intowebdriverio:v7from
sriteja777:ob_jasmine_fix_build_v7

Conversation

@sriteja777
Copy link
Contributor

@sriteja777 sriteja777 commented Apr 12, 2023

Proposed changes

  • Add support for Jasmine sessions to be captured by BrowserStack Test Observability
  • Add a performance tester utility for measuring performance of Observability related code.

Related v8 PR: #10013

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 marked this pull request as draft April 12, 2023 08:45
@sriteja777
Copy link
Contributor Author

@christian-bromann Please do not review / release this as of now. Will comment once final things are done

return true
}

return false
Copy link
Contributor

Choose a reason for hiding this comment

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

if check regarding framework === 'jasmine' not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. For jasmine, we are doing via reporter not through insights_handler. So for jasmine this will not get called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack

retries: { limit: testStats.retries || 0, attempts: testStats.retries || 0 }
}

if (eventType == 'TestRunStarted' || eventType == 'TestRunSkipped') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this check specifically for these 2 events?

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 adding the integrations object here. It is only required for new tests right? That's what we used to do in insight_handler before also.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this for TestRunFinished event also in case a user spawns driver inside the test itself in which case currently we would not be sending any integrations data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

test_run: testData
}
if (eventType == 'TestRunSkipped') {
eventType = 'TestRunFinished'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we overriding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have used TestRunSkipped just to handle some conditions or things to do when tests are skipped. But ultimately, this needs to be send to backend as TestRunFinished. So overriding the TestRunSkippedevent back to TestRunFinished

Copy link
Contributor

Choose a reason for hiding this comment

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

This param is being passed from onTestSkip callback of reporter. Would we not miss out on actually skipped tests by doing this? Please confirm this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So for those tests also where are called from onTestSkip, we were previously also sending it as a TestRunFinished event right?

Copy link
Contributor

Choose a reason for hiding this comment

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

So the test status is being sent as skipped and that reflects on reporting dashboard properly?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we send the event_type for skipped tests in other SDKs?

…rformance_testing

# Conflicts:
#	packages/wdio-browserstack-service/src/insights-handler.ts
#	packages/wdio-browserstack-service/src/reporter.ts
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.

Looks good to me, as soon as @amaanbs comments have been addressed and tests are passing we can merge.

sriteja777 and others added 9 commits April 28, 2023 20:15
# Conflicts:
#	packages/wdio-browserstack-service/src/insights-handler.ts
#	packages/wdio-browserstack-service/src/launcher.ts
#	packages/wdio-browserstack-service/src/reporter.ts
#	packages/wdio-browserstack-service/src/util.ts
@amaanbs
Copy link
Contributor

amaanbs commented May 3, 2023

Couple of things,

  1. Have we communicated internally with the team that we're now going to support generation of performance reports?
  2. Can we have some numbers on the performance impact of the performance tester class?

@sriteja777
Copy link
Contributor Author

  1. No will post, but that shouldn't be an approval for blockers right?
  2. The performance utility will be run only when we need. Why do we need numbers for that?

@amaanbs
Copy link
Contributor

amaanbs commented May 3, 2023

  1. We can't go ahead with this if internally any issue is raised by the team over exposing this functionality.
  2. Aligned, it's an optional utility so no need for performance numbers.

@sriteja777 sriteja777 marked this pull request as ready for review May 21, 2023 16:35
@christian-bromann
Copy link
Member

Is this ready for merge?

@sriteja777
Copy link
Contributor Author

Yes @christian-bromann

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.

👍

@christian-bromann christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label May 22, 2023
@christian-bromann christian-bromann merged commit c1dd107 into webdriverio:v7 May 22, 2023
@sourav-kundu
Copy link
Contributor

@christian-bromann When can we expect a release on v7 next?

@sourav-kundu
Copy link
Contributor

@christian-bromann Can we expect a v7 version release anytime soon?
The last release was 1.5 months ago

@christian-bromann
Copy link
Member

I haven't been able to look at v7 yet. If you like to help get the release out, it would be nice to get the unit test failures sorted which likely got introduced through a dependency update.

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.

4 participants