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

Support parallel test execution #609

Merged
merged 81 commits into from
May 28, 2018
Merged

Support parallel test execution #609

merged 81 commits into from
May 28, 2018

Conversation

doronpr
Copy link
Contributor

@doronpr doronpr commented Mar 6, 2018

  • fixed merge issues
  • fixed missing coverage warnings

Copy link
Member

@rotemmiz rotemmiz left a comment

Choose a reason for hiding this comment

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

Were you able to attach to debugger when using jest as test runner ?

"setupTestFrameworkScriptFile" : "./helpers/init.js",
"bail": true,
"verbose": true,
"forceExit": true
Copy link
Member

Choose a reason for hiding this comment

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

We should validate Detox doesn't leave anything open, and that everything is being closed correctly.
Please remove forceExit

"testEnvironment": "node",
"bail": true,
"verbose": true,
"forceExit": true
Copy link
Member

Choose a reason for hiding this comment

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

We should validate Detox doesn't leave anything open, and that everything is being closed correctly.
Please remove forceExit

cp.execSync(command, {stdio: 'inherit'});
}

function runJest() {
const currentConfiguration = config.configurations && config.configurations[program.configuration];
const maxTestWorkers = _.get(currentConfiguration, 'maxTestWorkers', 1);
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same way to get configuration on all variables, either move all to _.get, use getConfigFor, or "vanilla"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
};

const writeLockedDevices = lockedDevices => fs.writeFileSync(LOCK_FILE, JSON.stringify(lockedDevices));
Copy link
Member

Choose a reason for hiding this comment

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

please use functions instead of consts when declaring functions ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const stdout = _.get(result, 'stdout');
const runtimes = JSON.parse(stdout);
const newestRuntime = _.maxBy(runtimes.runtimes, r => Number(r.version));
if (newestRuntime) {
Copy link
Member

Choose a reason for hiding this comment

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

What happens if there no newestRuntime ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. added handling


async _createDeviceIfNecessary ({deviceIds, deviceType}) {
const numberOfDevicesNeededToCreate = Math.max(this.maxTestRunners - deviceIds.length, 0);
_.times(numberOfDevicesNeededToCreate, async () => await this.createDevice(deviceType));
Copy link
Member

Choose a reason for hiding this comment

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

if xcrun simctl supports it, we may be able to create all devices simultaneously and use Promise.all to wait for all responses instead of doing them serially.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure. It's Xcode after all.

Copy link
Contributor

@LeoNatan LeoNatan left a comment

Choose a reason for hiding this comment

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

What was changed in Earl Grey?

"react": "16.2.0",
"react-native": "0.53.3"
"react": "16.0.0",
"react-native": "0.51.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

iOS RN 53 support is supposedly broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

In what way?

@LeoNatan
Copy link
Contributor

LeoNatan commented Mar 6, 2018

Also, if possible, I would like to understand what the strategy for device creation and reuse is. Perhaps you've discussed it, but I am not in the know.

const newestRuntime = _.maxBy(runtimes.runtimes, r => Number(r.version));
if (newestRuntime) {
console.log('Creating simulator', `create "${type}" "${type}" "${newestRuntime.identifier}"`)
await this._execSimctl({cmd: `create "${type}" "${type}" "${newestRuntime.identifier}"`});
Copy link
Member

Choose a reason for hiding this comment

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

name should not be the type:

const name = `detox_${type}_${count}`
await this._execSimctl({cmd: `create "${name}" "${type}" "${newestRuntime.identifier}"`});

Now, when you change the name, you won't be able to query applesimutils by name anymore. @LeoNatan added query byType and byId. please check the implementation in:
https://github.com/wix/detox/compare/parallelSupport#diff-2ec04c0d120ceb57a189afbc6a59a9e8R27
https://github.com/wix/detox/compare/parallelSupport#diff-2ec04c0d120ceb57a189afbc6a59a9e8R39

}

async getDevice(deviceType) {
await retry(() => plockfile.lockSync(LOCK_FILE));
Copy link
Member

Choose a reason for hiding this comment

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

we may want to add Number.MAX_SAFE_INTEGER as retry amount, and reduce the exponential backoff timeouts to minimum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const plockfile = require('proper-lockfile');
const _ = require('lodash');
const retry = require('../utils/retry');
const LOCK_FILE = './device.registry.state.lock';
Copy link
Member

Choose a reason for hiding this comment

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

To support multiple Detox runs (e.g. in a non virtualized CI machine) LOCK_FILE should be added globally. We already create a ~/Library/Detox/ when compiling the framework, I think LOCK_FILE should be there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@rotemmiz rotemmiz merged commit 94348fc into master May 28, 2018
@rotemmiz rotemmiz deleted the test-parallelization branch May 28, 2018 21:40
@rotemmiz
Copy link
Member

Still missing - DOCS!

@rotemmiz
Copy link
Member

@doronpr, time to stress test some build agents.

@rotemmiz rotemmiz changed the title test sharding/parallelization v2 Support parallel test execution May 28, 2018
@wix wix locked and limited conversation to collaborators Jul 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants