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

Init framework before browser #4531

Merged

Conversation

@mgrybyk
Copy link
Member

mgrybyk commented Sep 25, 2019

Proposed changes

Init test framework before starting webdriver session.

(initial) purpose

Filter specs before webdriver session is started which is required to speedup tests execution.
Starting/stopping webdriver session takes some time for browsers and takes lots of time with Appium.
See #4326

NOT a breaking change

  • Cucumber users are not affected at all. Filtering just works out of the box.
  • Feature is disabled by default for Mocha/Jasmine users to avoid breaking changes (details below)

Feature Flags

Added featureFlags to wdio.conf that can have specFiltering property.
Example:

// by default enabled for Cucumber users and disabled for Mocha/Jasmine users.
featureFlags: {
    specFiltering: true
}

Spec filtering for Mocha/Jasmine users

Only if the feature is enabled with feature flag!

  • Mocha/Jasmine users have to move all required files that interacts with browser object from mochaOpts.require to before hook
  • Mocha/Jasmine users have to move all required files like transpilers from before hook to mochaOpts.require/jasmineNodeOpts.requires

some common cases

  • it is still possible to do something like this thanks to browser stub:
if (browser.isMobile) {
  it(browser.capabilities.browserName + ' test', () => {})
}
  • it's not possible to interact with browser in a root scope until the session is created. Code like browser.addCommand('foo', () => {}) have to be moved to helper files and called in before hook
  • If the feature is enabled but spec filtering has failed for some reason, test run proceeds as before, like there is no spec filtering. User can see more details in logs.

jasmineNodeOpts

Added support of built-in requires and helpers to be able to load compilers or helper files that are required to run tests.

Flow changes

current flow

  1. start browser
  2. run test framework:
    2.1. init test framework
    2.2. call before hook that expects browser object to be available
    2.3. run test framework

new flow

  1. create browser stub with capabilities and env flags (like isMobile)
  2. init test framework
  3. start browser
  4. call before hook
  5. run test framework

acknowledgement

  • only capabilities provided by user exist in browser.capabilities if spec filtering is enabled for Mocha/Jasmine before browser session is started. As a result user won't be able to use some runtime capabilities for tests filtering, like browser.capabilities.chrome.chromedriverVersion.
  • unable to detect isW3C and isSeleniumStandalone flags before browser session is started.

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
  • implement spec filtering for cucumber
  • implement spec filtering for mocha
  • implement spec filtering for jasmine
  • hide/change interface message about ignored spec file
  • unit tests
  • smoke tests
  • docs

Further comments

I think having this feature we can think on tags management for Jasmine and Mocha #2859

Cucumber has its own tag management system, so there is nothing to do for us

Reviewers: @webdriverio/technical-committee

@mgrybyk mgrybyk requested a review from webdriverio/project-committers Sep 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Sep 25, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4531      +/-   ##
=========================================
+ Coverage   99.39%   99.4%   +0.01%     
=========================================
  Files         207     209       +2     
  Lines        5306    5406     +100     
  Branches     1146    1177      +31     
=========================================
+ Hits         5274    5374     +100     
  Misses         29      29              
  Partials        3       3
Impacted Files Coverage Δ
packages/webdriver/src/utils.js 100% <ø> (ø) ⬆️
packages/wdio-cucumber-framework/src/utils.js 100% <ø> (ø) ⬆️
packages/wdio-config/src/constants.js 100% <ø> (ø) ⬆️
packages/wdio-cli/src/launcher.js 94.36% <ø> (-0.04%) ⬇️
packages/wdio-jasmine-framework/src/index.js 100% <100%> (ø) ⬆️
packages/webdriverio/src/index.js 100% <100%> (ø) ⬆️
packages/devtools/src/index.js 100% <100%> (ø) ⬆️
packages/webdriverio/src/protocol-stub.js 100% <100%> (ø)
packages/wdio-cucumber-framework/src/index.js 98.8% <100%> (+0.09%) ⬆️
packages/wdio-utils/src/envDetector.js 100% <100%> (ø)
... and 9 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 902ea1f...72c9865. Read the comment docs.

Copy link
Member

christian-bromann left a comment

I think this solution would work. I am wondering if before should be beforeFramework and we find a different name for the before. This could avoid the breaking change for Mocha and Jasmine.

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Sep 26, 2019

@christian-bromann we can't do this because browser object should be available in before hook, otherwise it's gonna be a breaking change not just for frameworks but for services as well.

There is a tricky situation with Mocha and Jasmine.
In order to filter spec files I need to load them first. In order to load spec files I need to load transpilers and helper files.
Quite often helper files may depend on browser object that is not available before starting up webdriver session.

Some users (like me) may use require to load not just transpilers but also helper files.
Example:

    mochaOpts: {
        require: [ 'tsconfig-paths/register', './src/wdio/commands' ]
    },

It won't work anymore because browser object is not available at that point.

So, to make filtering work in Mocha/Jasmine we need users to pass helper files required by tests that are not dependant on browser to mochaOpts.require or beforeFramework hook.
Other files should be required in before hook or be adopted somehow to no rely on browser.

What else can be done?

  • We can try load specs, log some warning on failure and proceed test execution as it was before (without filtering) (completed).
  • The feature can be disabled by default for Mocha and Jasmine users.
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Sep 26, 2019

@christian-bromann here is something that will no longer work for Mocha / Jasmine users who want to use filtering feature:

example:

// browser is not available here!
if (browser.isMobile) {
  describe("MY suite 2", () => {
    it("my test 002", () => {
       // but it is available here as before
       browser.url("http://guinea-pig.webdriver.io")
    })
  })
}

Another example when helper file should be moved to before hook because browser object is not available earlier.

browser.addCommand("getLink", function () {
    return this.optAttribute("href")
}, true)
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Sep 26, 2019

  • there is no RUNNING/PASSED message for skipped spec;
  • there is a message in log saying that spec is skipped
    2019-09-26T14:06:46.810Z INFO @wdio/cli: [0-2] SKIPPED in chrome - /test/specs/_debug/my-e2e-3.spec.ts
  • skipped specs amount is printed in the end if there are such;

image

@christian-bromann

This comment has been minimized.

Copy link
Member

christian-bromann commented Sep 27, 2019

This will be a breaking change and we should release it as part of v6 including dropping support for Node v8 (LTS ends at the end of this year). I will create a label for it. I wonder if we should drop support earlier to release this.

@wswebcreation

This comment has been minimized.

Copy link
Member

wswebcreation commented Sep 27, 2019

Wow, this is amazing and a big changer!

Awesome work already!

@christian-bromann christian-bromann added this to the v6 milestone Sep 27, 2019
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Sep 27, 2019

@christian-bromann we can make it as an optional feature for mocha and jasmine users, and enabled by default for Cucumber users.
Why do you think it is breaking? (I might be missing something, sorry)

@christian-bromann

This comment has been minimized.

Copy link
Member

christian-bromann commented Sep 27, 2019

This fact that this is not working anymore:

// browser is not available here!
if (browser.isMobile) {

I would be fine to only introduce it in Cucumber for now and create an issue to introduce it in Mocha and Jasmine when we move to v6

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Sep 27, 2019

@christian-bromann it's configurable.
If feature is enabled it won't work, if it's disabled it works as before

@jlin-namely

This comment has been minimized.

Copy link

jlin-namely commented Oct 1, 2019

@christian-bromann and @mgrybyk This is very useful feature. Is it possible to release this soon rather than waiting until v6? Thank you!

@mgrybyk mgrybyk force-pushed the mgrybyk:init-framework-before-browser branch from 3f1e6f3 to 84257ef Oct 2, 2019
@mgrybyk mgrybyk force-pushed the mgrybyk:init-framework-before-browser branch from 84257ef to 045fdb6 Oct 3, 2019
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 3, 2019

@christian-bromann changed a bit how it works.

  • removed beforeFramework hook at all
  • created browser stub, an object that has capabilities and most of environment flags like isMobile

tested changes in my local project (with mocha and jasmine), it was very easy for me to adopt it, an example for mocha:
before:

    mochaOpts: {
        require: [
            "src/wdio/browserFlags", // my custom flags like isMobileEmulation
            "src/wdio/commands", // my custom commands
        ]
    },

after

    mochaOpts: {
        require: [
            "src/wdio/browserFlags", // my custom flags like isMobileEmulation
        ]
    },
    before () {
        require("../src/wdio/commands") // my custom commands
    },
    featureFlags: {
        specFiltering: true // enable spec filtering feature
    }

It is now possible to use browser.capabilities.browserName in test names of filter specs depending on env flag like browser.isMobile.

It's still a POC and some functionality is missing

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 3, 2019

also had to add jasmine's require/helpers support to avoid adding beforeFramework hook:

    jasmineNodeOpts: {
        requires: [
            "src/wdio/browserFlags",
        ],
    },
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 4, 2019

I think the feature is more or less ready now. It won't cause any issues to existing users, it's pretty much easy to start using it for existing Mocha/Jasmine users (from my point of view).

The feature works for Cucumber users out of the box without any breaking changes.

Going to update the PR description to provide with more visibility

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 4, 2019

The PR is ready for initial review, will proceed with adding docs/etc if there are no objections

@mgrybyk mgrybyk marked this pull request as ready for review Oct 4, 2019
@martinfrancois

This comment has been minimized.

Copy link
Contributor

martinfrancois commented Oct 4, 2019

I have to agree with @wswebcreation, this is really great work!

I was wondering, since you mentioned:

Mocha/Jasmine users have to move all required files like transpilers from before hook to mochaOpts.require/jasmineNodeOpts.requires

Does having spec files in typescript and especially config files in typescript still work?

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 4, 2019

@martinfrancois there are no changes in this area at all

@martinfrancois

This comment has been minimized.

Copy link
Contributor

martinfrancois commented Oct 4, 2019

@mgrybyk thanks for the confirmation! Was just unsure since you need to add require('ts-node') in the before hook to use typescript ;) and I would kind of consider typescript to be a transpiler as well here, but I guess you mainly meant babel then

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Oct 4, 2019

It still work as is if the feature is not enabled for mocha and jasmine users. If the feature is enabled ts node have to be moved to require array in mochaOpts / jasmineNodeOpts

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Nov 4, 2019

UPD: had to update types after rebase

@mgrybyk mgrybyk force-pushed the mgrybyk:init-framework-before-browser branch from 9e2519d to 72c9865 Nov 5, 2019
@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Nov 5, 2019

rebased

@christian-bromann do you have some more questions / suggestions / tasks to do here?

Copy link
Member

christian-bromann left a comment

Nope, we can ship it. I think this is a great first step into the right direction. I am still not sure what I feel about the protocol stub. I still think it makes things more complicated as they have to be but I don't have any better solution to provide users with data on capabilities so they can dynamically create test names.\

Thank you for the PR. Great work!

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Nov 5, 2019

Me neither without introducing certain limitations mentioned above.
Well if jasmine and mocha can filters tests same way Cucumber do...

So, we can merge it you think @christian-bromann ?

If so, let's do it!

@christian-bromann christian-bromann merged commit bd0424c into webdriverio:master Nov 5, 2019
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 99.39%)
Details
codecov/project 99.4% (+0.01%) compared to 902ea1f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@jlin-namely

This comment has been minimized.

Copy link

jlin-namely commented Nov 5, 2019

Very much appreciated for work @mgrybyk
This is a highly valuable problem that we have for cucumber framework currently.
Just wondering when it will be released?

@christian-bromann

This comment has been minimized.

Copy link
Member

christian-bromann commented Nov 6, 2019

Just wondering when it will be released?

It was released with v5.16.0

@nami-varthakavi

This comment has been minimized.

Copy link
Contributor

nami-varthakavi commented Nov 14, 2019

I'm having an issue registering jasmine-expect in the jasminenodeOpts, this is how I register it

jasmineNodeOpts: {
    defaultTimeoutInterval: 60000,
    requires: ['jasmine-expect']
}

Can someine help me identify what's the issue behind it then

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Nov 14, 2019

@nami-varthakavi you may ping me in gitter. What problem do you have? It works well for me, thanks for the package, btw :)

added jasmine-expect same as you to requires, then tried in my test expect('').toBeIso8601() and it works: Expected '' to be iso8601.

@nami-varthakavi

This comment has been minimized.

Copy link
Contributor

nami-varthakavi commented Nov 14, 2019

@nami-varthakavi you may ping me in gitter. What problem do you have? It works well for me, thanks for the package, btw :)

added jasmine expect same as you to required, then tried in my test expect('').toBeIso8601() and it works: Expected '' to be iso8601.

Let me try that again, would you mind replying me in the gitter.

@nami-varthakavi

This comment has been minimized.

Copy link
Contributor

nami-varthakavi commented Nov 14, 2019

expect('').toBeIso8601()

I get this error: TypeError: expect(...).toBeIso8601 is not a function

@nami-varthakavi

This comment has been minimized.

Copy link
Contributor

nami-varthakavi commented Nov 14, 2019

@nami-varthakavi you may ping me in gitter. What problem do you have? It works well for me, thanks for the package, btw :)

added jasmine expect same as you to required, then tried in my test expect('').toBeIso8601() and it works: Expected '' to be iso8601.

is it required / requires

@Stejnar

This comment has been minimized.

Copy link

Stejnar commented Nov 15, 2019

I can not get the specFiltering option working with mocha and typescript. Any documentation on how to achieve this would be nice.

It throws a SyntaxError from my .ts spec files. Converting them to javascript works, but is not an option.

Do I have to change any require of ts-node or tsconfig-paths/register? I am currently doing it like this

@mgrybyk

This comment has been minimized.

Copy link
Member Author

mgrybyk commented Nov 15, 2019

@Stejnar

This comment has been minimized.

Copy link

Stejnar commented Nov 15, 2019

Thanks for the link. I assume you count the typescript compilation as transpiler here? If so, there is no difference between my config and your example.
As I stated, removing typescript from the equation resolves the error, so this might be solely typescript related. I will investigative further and open an issue if I find something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.