Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrap test function with before after step hooks #4354

Merged
merged 10 commits into from
Sep 27, 2019
Merged

Wrap test function with before after step hooks #4354

merged 10 commits into from
Sep 27, 2019

Conversation

mgrybyk
Copy link
Member

@mgrybyk mgrybyk commented Aug 12, 2019

NOTE

Please bump version of all the packages when doing release!

Problem

Currently we use mocha/jasmine before/afterEach hooks as wdio before/afterTest hooks.
This has certain limitations, for example if test looks like this

describe('suite', () => {
  describe('sub suite', () => {
    it('test', () => {
      // some test
    })

    afterEach(() => {
      // reload page
    })
  })
})

there is no way to capture screenshot, because screenshot is taken after afterEach hook.

Proposed changes

  • Wrap mocha/jasmine framework functions (it, beforeEach and others) with wdio hooks like this:
global.it = function (..args) {
  // call wdio before test/hook
  // call original `it`/`afterEach`/etc function
  // call wdio after test/hook
}

fixes #4004

  • Additionally mocha users can now access mocha runnable context that adds more flexibility for custom reporters (ex. Mochawesome with screenshots)
afterTest: function (test, context) {
    context // { test, _runnable }
},

same for hooks

afterHook: function (test, context) {
    context // { test, _runnable, currentTest }
},
  • have same approach for all test frameworks: mocha, jasmine and cucumber. See file packages/wdio-utils/src/test-framework/testFnWrapper.js

  • allow mixing async and sync like this:

it('async test', async () => {
  await browser.pause(1)
})

it('sync test' () => {
  browser.pause(1) // it was Promise before
})

Types of changes

  • Bugfix (non-breaking change which fixes an issue)

there might be some regressions for mocha/jasmine users that strongly rely on beforeTest/afterTest in some very specific way, sorry to say I might miss some use cases, it should be easy to deal with them.
Cucumber users are not affected.

TODO

  • have same approach to run before/after test hooks for all frameworks (mocha, cucumber, jasmine)
  • verify before/after test hooks in async mode
  • add @wdio/sync unit tests
  • verify before/afterTest hook functions signature (ideally to avoid breaking changes)
  • add smoke tests
  • update before/after hook signature in configs and types
  • avoid adding dependencies to @wdio/utils
  • reorganize smoke added test

Reviewers: @webdriverio/technical-committee

@mgrybyk mgrybyk added work-in-progress PR: Bug Fix 🐛 PRs that contain bug fixes labels Aug 12, 2019
This was referenced Aug 12, 2019
@codecov
Copy link

codecov bot commented Aug 12, 2019

Codecov Report

Merging #4354 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4354      +/-   ##
==========================================
+ Coverage   99.28%   99.29%   +0.01%     
==========================================
  Files         188      192       +4     
  Lines        4765     4856      +91     
  Branches     1020     1039      +19     
==========================================
+ Hits         4731     4822      +91     
  Misses         31       31              
  Partials        3        3
Impacted Files Coverage Δ
packages/webdriverio/src/index.js 100% <ø> (ø) ⬆️
packages/wdio-sync/src/utils.js 100% <ø> (ø) ⬆️
packages/webdriverio/src/utils/getElementObject.js 97.82% <ø> (ø) ⬆️
packages/wdio-repl/src/index.js 100% <ø> (ø) ⬆️
packages/webdriverio/src/utils/Timer.js 96.22% <ø> (ø) ⬆️
packages/webdriverio/src/multiremote.js 100% <ø> (ø) ⬆️
packages/wdio-cucumber-framework/src/utils.js 100% <ø> (ø) ⬆️
packages/wdio-utils/src/shim.js 100% <100%> (ø)
packages/wdio-jasmine-framework/src/index.js 100% <100%> (ø) ⬆️
...ges/wdio-utils/src/test-framework/testFnWrapper.js 100% <100%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed8a8d2...d672285. Read the comment docs.

*
* @param {object} wrapFunctions executeHooksWithArgs, executeAsync, runSync
* @param {string} type Test/Step or Hook
* @param {object} spec specFn and specFnArgs
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure it makes much sense to pass spec, before, after as objects

@mgrybyk mgrybyk marked this pull request as ready for review September 18, 2019 14:15
Copy link
Contributor

@CrispusDH CrispusDH left a comment

Choose a reason for hiding this comment

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

Tiny comments

@mgrybyk mgrybyk closed this Sep 20, 2019
@mgrybyk mgrybyk reopened this Sep 20, 2019
CrispusDH
CrispusDH previously approved these changes Sep 20, 2019
@christian-bromann
Copy link
Member

Please remove the work-in-progress label if it is ready for review

@mgrybyk
Copy link
Member Author

mgrybyk commented Sep 20, 2019

PR is ready for 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.

I am a bit worried to add a dependency to @wdio/utils given that the package should contain minimal helper functions that don't require a dependency to a different sub package. This can lead to a circular dependency issue. Do you think we can reorg this?

packages/wdio-config/src/shim.js Show resolved Hide resolved
packages/wdio-config/src/shim.js Show resolved Hide resolved
packages/wdio-cucumber-framework/tests/adapter.test.js Outdated Show resolved Hide resolved
packages/wdio-cucumber-framework/tests/adapter.test.js Outdated Show resolved Hide resolved
packages/wdio-jasmine-framework/src/index.js Show resolved Hide resolved
packages/wdio-mocha-framework/src/index.js Show resolved Hide resolved
packages/wdio-utils/package.json Outdated Show resolved Hide resolved
tests/mocha/service.js Outdated Show resolved Hide resolved
tests/helpers/config.js Outdated Show resolved Hide resolved
tests/mocha/service.js Outdated Show resolved Hide resolved
@mgrybyk
Copy link
Member Author

mgrybyk commented Sep 23, 2019

@christian-bromann updated PR according to comments

@mgrybyk mgrybyk closed this Sep 23, 2019
@mgrybyk mgrybyk reopened this Sep 23, 2019
@mgrybyk
Copy link
Member Author

mgrybyk commented Sep 24, 2019

added more unit tests and removed runFnInFiberContextWithCallback from @wdio/sync as far as it is not used anywhere

@mgrybyk mgrybyk closed this Sep 24, 2019
@mgrybyk mgrybyk reopened this Sep 24, 2019
@mgrybyk mgrybyk closed this Sep 24, 2019
@mgrybyk mgrybyk reopened this Sep 24, 2019
@mgrybyk mgrybyk closed this Sep 24, 2019
@mgrybyk mgrybyk reopened this Sep 24, 2019
@mgrybyk mgrybyk closed this Sep 25, 2019
@mgrybyk mgrybyk reopened this Sep 25, 2019
@mgrybyk
Copy link
Member Author

mgrybyk commented Sep 25, 2019

@christian-bromann rebased, added temporary pac-resolver mock f5ae88d to prove that all tests are passing

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.

some more comments

packages/wdio-config/package.json Outdated Show resolved Hide resolved
packages/wdio-config/tests/shim-sync.test.js Outdated Show resolved Hide resolved
packages/wdio-sync/src/wrapCommand.js Show resolved Hide resolved
@mgrybyk
Copy link
Member Author

mgrybyk commented Sep 27, 2019

@christian-bromann updated PR according to comments.

Please note that due to lot's of changes in imports I'd recommend to release all the packages after merging this one

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.

This is a great refactoring effort , well done 👏

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.

WDIO hook for it blocks
3 participants