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
merged 11 commits into from Nov 5, 2019
Merged

Init framework before browser #4531

merged 11 commits into from Nov 5, 2019

Conversation

mgrybyk
Copy link
Member

@mgrybyk 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 added work-in-progress PR: Breaking Change 💥 PRs that contain breaking changes PR: New Feature 🚀 PRs that contain new features labels Sep 25, 2019
@mgrybyk mgrybyk requested a review from a team September 25, 2019 22:58
@codecov
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 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 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
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
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
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
Copy link
Member

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
Copy link
Member

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
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
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
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
Copy link

@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 removed the PR: Breaking Change 💥 PRs that contain breaking changes label Oct 2, 2019
@mgrybyk
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
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
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
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 October 4, 2019 11:58
@martinfrancois
Copy link
Contributor

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
Copy link
Member Author

mgrybyk commented Oct 4, 2019

@martinfrancois there are no changes in this area at all

@martinfrancois
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
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

@martinfrancois
Copy link
Contributor

martinfrancois commented Oct 4, 2019

That's actually what I was worried about, when using typed configurations I was wondering if we enable this feature with mocha / jasmine via the feature toggles if it would work, since there you can't move the require('ts-node') to the mochaOpts / jasmineNodeOpts, as you need to initialize require("ts-node/register") before the configuration is even initialized. That wouldn't be a problem?

@mgrybyk
Copy link
Member Author

mgrybyk commented Oct 5, 2019

No, typed configuration is not affected. If ts node is in js file that requires typed config there is no need to require it again anywhere else

@mgrybyk
Copy link
Member Author

mgrybyk commented Nov 15, 2019

@ArminBu
Copy link
Contributor

ArminBu 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.

@mgrybyk mgrybyk deleted the init-framework-before-browser branch November 23, 2019 14:11
@karthimathan
Copy link

Hi Team

I am using cucumber and webdriverio framework,
In my project i have created two spec file eg: (Spec1 and Spec2),
while running the spec file execution "deleteSession()" is called after the execution of each spec file Then next spec again "Initiate new session", is this fixed in to "5.18.7"

how can avoid deleting the session for each spec run?

Please refer my package version below
"webdriverio": "^5.18.7",
"cucumber": "^6.0.5"

@mgrybyk
Copy link
Member Author

mgrybyk commented Mar 3, 2020

how can avoid deleting the session for each spec run?

There is no way to do it with cucumber framework

@karthimathan
Copy link

karthimathan commented Mar 4, 2020

how can avoid deleting the session for each spec run?

There is no way to do it with cucumber framework
@mgrybyk Thanks for your replay

What is the best way to run multiple feature file with one session in cucumber framework with wdio ?
Previously we have used selenium driver we can start and stop the session manually
Is there any way to start and stop session manually in cucumber and wdio framework?
Any help appreciated for your side

@mgrybyk
Copy link
Member Author

mgrybyk commented Mar 4, 2020

@karthimathan it's not possible with current runner and cucumber. If you like you can run wdio programmatically and use cucumber js.

Why do you need to keep session?

@karthimathan
Copy link

@karthimathan it's not possible with current runner and cucumber. If you like you can run wdio programmatically and use cucumber js.

Why do you need to keep session?

@mgrybyk Thanks for your replay

We are using cucumber js only to run our test scripts

Sample test case:

Given App is loaded
When I click on Menu
Then the screen is loaded

Step Definition:
//using typescript to define steps Ex:
Given("App is loaded", () => {
)

We have wdio integrated with the framework and have define two config files 1. Commonconfig.js (wdio capabilities) 2. Device specific config file (Appium capabilities)

We run the below command to execute:
./node_modules/.bin/wdio ./wdio_config/mob-wdio.conf.js

Is this expected or should we modify anything here

Why do you need to keep session?
We have multiple UI screens and we have defined each under different feature files. We want to run all the feature files in one session(i.e, without closing the app)

@alfonso-presa
Copy link
Member

@mgrybyk @christian-bromann I've been looking at this PR to see if it solves our use case. We have cucumberjs and it's said here that specFiltering is enabled in cucumber and it has it's own tag mechanism.... The problem I'm seeing is that we have scenarios that should be skipped in some browsers and devices (i.e. firefox and ie, only in firefox or only in edge, only in chrome in android, .... lot's of combinations, you get the point ;-) ). With cucumber pickle filtering we can exclude upfront (prior to execution) the tests we want, but we cannot make this filtering dynamic depending on the browser/device AFAIK. We have implemented a cucumber hook that skips the scenario/feature depending on the tags, but session is still started. Is there any way we can apply the same dynamic filtering that has been enabled in mocha or jasmine to cucumber tests? We don't want to have to execute several wdio commands for each capability combination to be able to inject the specific exclude tag for each browser/device....

@alfonso-presa
Copy link
Member

I've been able to do it by adding my own framework extending cucumber one... But wouldn't it be worth adding some kind of extension point (may be a hook) so that people can filter tests?

class CucumberAdapter extends CucumberFramework.CucumberAdapter {
  testCases: {
    pickle: any,
    uri: string
  }[] = [];

  constructor(...args) {
    super(...args);
  }

  async init () {
    const result = await super.init();
    this.testCases = this.testCases.filter(scenario => filter(scenario, (global as any).browser, process.env));
    return result;
  }

  hasTests () {
    return this.testCases.length > 0;
  }

  run () {
    return super.run();
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 PRs that contain new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet