theintern / intern

A next-generation code testing stack for JavaScript.
https://theintern.io/
Other
4.36k stars 310 forks source link

WebDriver capabilities should merge by default #840

Closed jason0x43 closed 6 years ago

jason0x43 commented 6 years ago

In the past, user-provided webdriver capabilities would merge with Intern's own default capabilities. Currently they're being overwritten unless capabilities are explicitly merged with capabilities+. We should probably restore the original behavior.

bryanforbes commented 6 years ago

I've also found another strange behavior with "capabilities+" when extending from another file:

a.json:

{
    "capabilities+": {
        "fixSessionCapabilities": false
    },

    "configs": {
        "browserstack": {
            "tunnel": "browserstack",

            "capabilities+": {
                "browserstack.debug": false
            }
        }
    }
}

b.json:

{
    "extends": "./a.json",

    "capabilities+": {
        "project": "My Project",
        "name": "Thing"
    }
}

Output from intern showConfig config=b.json@browserstack:

{
    "bail": false,
    "basePath": "/Users/bryan/Projects/dojo2/loader/",
    "baseline": false,
    "benchmark": false,
    "browser": {
        "plugins": [],
        "reporters": [],
        "suites": []
    },
    "capabilities": {
        "browserstack.debug": false,
        "fixSessionCapabilities": false,
        "name": "intern"
    },
    "coverage": [],
    "coverageVariable": "__coverage__",
    "debug": false,
    "defaultTimeout": 30000,
    "environments": [
        {
            "browserName": "node"
        }
    ],
    "filterErrorStack": false,
    "functionalCoverage": true,
    "functionalSuites": [],
    "functionalTimeouts": {
        "connectTimeout": 30000
    },
    "grep": {},
    "instrumenterOptions": {},
    "internPath": "/Users/bryan/Projects/dojo2/loader/node_modules/intern/",
    "loader": {
        "script": "default"
    },
    "maxConcurrency": null,
    "name": "node",
    "node": {
        "plugins": [],
        "reporters": [],
        "suites": []
    },
    "plugins": [],
    "reporters": [
        {
            "name": "runner"
        }
    ],
    "runInSync": false,
    "serveOnly": false,
    "serverPort": 9000,
    "serverUrl": "http://localhost:9000/",
    "sessionId": "",
    "showConfig": true,
    "socketPort": 9001,
    "tunnel": "browserstack",
    "tunnelOptions": {
        "tunnelId": "1510687879336"
    }
}

Not only are the capabilities from a.json not merged with the defaults, but the capabilities+ from b.json are not merged with the capabilities from a.json.

jason0x43 commented 6 years ago

So...what's happening there is kind of interesting. Config processing happens in 2 stages: in the first part, some code in common/util.ts loads the config and any parent configs, mixing options together along the way. This code isn't particularly smart and doesn't validate any options, it just mixes property values. It breaks when you have 'capabilities+' in both a parent and a child, because it treats 'capabilities+' as a name, so the value from a child config will overwrite the value of a parent that also uses 'capablities+'. (The assumption is that a base config would use 'capabilities' and a child would use 'capabilities+' to extend it.)

jason0x43 commented 6 years ago

The config processing logic has been moved out of the executors, allowing it to work during initial loading of config files.

jason0x43 commented 6 years ago

Hmmm...not happy with that, and there appear to be other merging issues that can crop up with complex setups.

jason0x43 commented 6 years ago

Never mind. Happy enough.