Skip to content

fix jasmine afterTest hook#11625

Merged
christian-bromann merged 2 commits intowebdriverio:mainfrom
tamil777selvan:fix-jasmine-hook
Nov 8, 2023
Merged

fix jasmine afterTest hook#11625
christian-bromann merged 2 commits intowebdriverio:mainfrom
tamil777selvan:fix-jasmine-hook

Conversation

@tamil777selvan
Copy link
Member

Proposed changes

Fixes #11495

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

*/
// @ts-expect-error
const _timeout = this?._runnable?._timeout || globalThis.jasmine?.DEFAULT_TIMEOUT_INTERVAL || timeout
const _timeout = (this?._runnable?._timeout || globalThis.jasmine?.DEFAULT_TIMEOUT_INTERVAL || timeout) - 3
Copy link
Member

Choose a reason for hiding this comment

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

Do we know that 3ms are enough for this to not happen? It seems a bit arbitrary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not need to be precisely 3ms. It just needs to be a shorter duration than the overall timeout. So, I've used 3ms, making sure it doesn't disrupt the timeouts set & execution timeline.

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 move the 3 to a const at the top of the file with a descriptive name and a comment as to why the value is chosen? I think it would greatly help anyone stumbling over it in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, on it.

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.

Awesome, LGTM 👍

@christian-bromann christian-bromann merged commit be47a15 into webdriverio:main Nov 8, 2023
@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Nov 8, 2023
@tamil777selvan tamil777selvan deleted the fix-jasmine-hook branch November 8, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Bug Fix 🐛 PRs that contain bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: afterTest hook contains wrong test object but correct error in a specific scenario

2 participants