Skip to content
New issue

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

#

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

Already on GitHub? # to your account

PR (Issue #1176) - Stop Forcing -gpu host on android emulators #1177

Merged
merged 2 commits into from
Mar 14, 2019
Merged

PR (Issue #1176) - Stop Forcing -gpu host on android emulators #1177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84ed099
#1176 - Updated android/Emulator.js to stop adding the -gpu host para…
RyanThomas73 Feb 26, 2019
3899ea2
#1176 - Added support for a --gpu cli parameter to explicit specify t…
RyanThomas73 Feb 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion detox/local-cli/detox-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ program
'Specify test file to run')
.option('-H, --headless',
'[Android Only] Launch Emulator in headless mode. Useful when running on CI.')
.option('--gpu [gpu mode]',
'[Android Only] Launch Emulator with the specific -gpu [gpu mode] parameter.')
.option('-w, --workers <n>',
'[iOS Only] Specifies number of workers the test runner should spawn, requires a test runner with parallel execution support (Detox CLI currently supports Jest)', 1)
.option('-n, --device-name [name]',
Expand Down Expand Up @@ -127,13 +129,14 @@ function runMocha() {
const screenshots = program.takeScreenshots ? `--take-screenshots ${program.takeScreenshots}` : '';
const videos = program.recordVideos ? `--record-videos ${program.recordVideos}` : '';
const headless = program.headless ? `--headless` : '';
const gpu = program.gpu ? `--gpu ${program.gpu}` : '';
const color = program.color ? '' : '--no-colors';
const deviceName = program.deviceName ? `--device-name "${program.deviceName}"` : '';

const debugSynchronization = program.debugSynchronization ? `--debug-synchronization ${program.debugSynchronization}` : '';
const binPath = path.join('node_modules', '.bin', 'mocha');
const command = `${binPath} ${testFolder} ${configFile} ${configuration} ${loglevel} ${color} ` +
`${cleanup} ${reuse} ${debugSynchronization} ${platformString} ${headless} ` +
`${cleanup} ${reuse} ${debugSynchronization} ${platformString} ${headless} ${gpu} ` +
`${logs} ${screenshots} ${videos} ${artifactsLocation} ${deviceName} ${collectExtraArgs()}`;

console.log(command);
Expand All @@ -154,6 +157,7 @@ function runJest() {
cleanup: program.cleanup,
reuse: program.reuse,
debugSynchronization: program.debugSynchronization,
gpu: program.gpu,
headless: program.headless,
artifactsLocation: program.artifactsLocation,
recordLogs: program.recordLogs,
Expand Down
15 changes: 12 additions & 3 deletions detox/src/devices/android/Emulator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,16 @@ class Emulator {
async boot(emulatorName) {
const emulatorArgs = _.compact([
'-verbose',
'-gpu', this.gpuMethod(),
'-no-audio',
argparse.getArgValue('headless') ? '-no-window' : '',
`@${emulatorName}`
]);

const gpuMethod = this.gpuMethod();
if(gpuMethod) {
emulatorArgs.push('-gpu', gpuMethod);
}

let childProcessOutput;
const tempLog = `./${emulatorName}.log`;
const stdout = fs.openSync(tempLog, 'a');
Expand Down Expand Up @@ -82,6 +86,11 @@ class Emulator {
}

gpuMethod() {
const gpuArgument = argparse.getArgValue('gpu');
if(gpuArgument) {
return gpuArgument;
}

if (argparse.getArgValue('headless')) {
switch (os.platform()) {
case 'darwin':
Expand All @@ -93,9 +102,9 @@ class Emulator {
default:
return 'auto';
}
} else {
return 'host';
}

return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@RyanThomas73 for backwards compatibility, please return host as the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d4vidi Returning host as the default reintroduces the problem where it will always include -gpu host with the emulator command preventing the emulator command from honoring an explicit gpu mode specific in the emulator config via hw.gpu.mode=

Copy link
Contributor Author

@RyanThomas73 RyanThomas73 Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We could choose a specific value to map to undefined so it uses the emulator config default (e.g. detox test .... --gpu default) but I feel its better the way it is in the PR now. The user shouldn't have to explicitly specify default to get the default behavior from the emulator.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, you are right - I was not aware of that.
Nevertheless the issue of back-compatibility still is concerning (keeping the community happy). That being said, I take it that the crucial use cases are of the CI, where typically emulators are run headless, which isn't affected in this case. So, bottom line here - I'll approve and merge.
Thank you :)

}
}

Expand Down