Skip to content

Commit 258674b

Browse files
rplrpl
committedJul 20, 2020
chore: Renamed cli option to --adb-remove-old-artifacts and some minor tweaks to the related tests
·
8.5.05.0.0
1 parent a5e335b commit 258674b

File tree

6 files changed

+131
-109
lines changed

6 files changed

+131
-109
lines changed
 

‎src/cmd/run.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ export type CmdRunParams = {|
4545
adbPort?: string,
4646
adbDevice?: string,
4747
adbDiscoveryTimeout?: number,
48-
adbCleanArtifacts?: boolean,
48+
adbRemoveOldArtifacts?: boolean,
4949
firefoxApk?: string,
5050
firefoxApkComponent?: string,
5151

@@ -88,7 +88,7 @@ export default async function run(
8888
adbPort,
8989
adbDevice,
9090
adbDiscoveryTimeout,
91-
adbCleanArtifacts,
91+
adbRemoveOldArtifacts,
9292
firefoxApk,
9393
firefoxApkComponent,
9494
// Chromium CLI options.
@@ -167,7 +167,7 @@ export default async function run(
167167
adbPort,
168168
adbBin,
169169
adbDiscoveryTimeout,
170-
adbCleanArtifacts,
170+
adbRemoveOldArtifacts,
171171

172172
// Injected dependencies.
173173
firefoxApp,

‎src/extension-runners/firefox-android.js

+19-27
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export type FirefoxAndroidExtensionRunnerParams = {|
7171
adbPort?: string,
7272
adbDevice?: string,
7373
adbDiscoveryTimeout?: number,
74-
adbCleanArtifacts?: boolean,
74+
adbRemoveOldArtifacts?: boolean,
7575
firefoxApk?: string,
7676
firefoxApkComponent?: string,
7777

@@ -385,30 +385,6 @@ export class FirefoxAndroidExtensionRunner {
385385
await adbUtils.amForceStopAPK(selectedAdbDevice, selectedFirefoxApk);
386386
}
387387

388-
async adbOldArtifactsDir(removeArtifacts?: boolean) {
389-
const {
390-
adbUtils,
391-
selectedAdbDevice,
392-
} = this;
393-
394-
const foundDirectories = await adbUtils.checkOrCleanArtifacts(
395-
selectedAdbDevice, removeArtifacts
396-
);
397-
398-
if (!foundDirectories) {
399-
return;
400-
}
401-
if (removeArtifacts) {
402-
log.info('Old web-ext artifacts have been found and removed ' +
403-
`from ${selectedAdbDevice} device`);
404-
} else {
405-
log.warn(
406-
`Old artifacts directories have been found on ${selectedAdbDevice} ` +
407-
'device. Use --adb-clean-artifacts to remove them automatically.'
408-
);
409-
}
410-
}
411-
412388
async adbCheckRuntimePermissions() {
413389
const {
414390
adbUtils,
@@ -456,6 +432,7 @@ export class FirefoxAndroidExtensionRunner {
456432
params: {
457433
customPrefs,
458434
firefoxApp,
435+
adbRemoveOldArtifacts,
459436
},
460437
} = this;
461438
// Create the preferences file and the Fennec temporary profile.
@@ -466,8 +443,23 @@ export class FirefoxAndroidExtensionRunner {
466443
customPrefs,
467444
});
468445

469-
//Checking for older artifacts
470-
await this.adbOldArtifactsDir(this.params.adbCleanArtifacts);
446+
// Check if there are any artifacts dirs from previous runs and
447+
// automatically remove them if adbRemoteOldArtifacts is true.
448+
const foundOldArtifacts = await adbUtils.detectOrRemoveOldArtifacts(
449+
selectedAdbDevice, adbRemoveOldArtifacts
450+
);
451+
452+
if (foundOldArtifacts) {
453+
if (adbRemoveOldArtifacts) {
454+
log.info('Old web-ext artifacts have been found and removed ' +
455+
`from ${selectedAdbDevice} device`);
456+
} else {
457+
log.warn(
458+
`Old artifacts directories have been found on ${selectedAdbDevice} ` +
459+
'device. Use --adb-remove-old-artifacts to remove them automatically.'
460+
);
461+
}
462+
}
471463

472464
// Choose a artifacts dir name for the assets pushed to the
473465
// Android device.

‎src/program.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -663,8 +663,8 @@ Example: $0 --help run.
663663
type: 'number',
664664
requiresArg: true,
665665
},
666-
'adb-clean-artifacts': {
667-
describe: 'Remove old artifact directories from the adb device',
666+
'adb-remove-old-artifacts': {
667+
describe: 'Remove old artifacts directories from the adb device',
668668
demandOption: false,
669669
type: 'boolean',
670670
},

‎src/util/adb.js

+16-17
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,9 @@ import {
88
} from '../errors';
99
import {createLogger} from '../util/logger';
1010

11-
const DEVICE_DIR_BASE = '/sdcard';
12-
const ARTIFACTS_DIR_PREFIX = '/web-ext-artifacts';
11+
export const DEVICE_DIR_BASE = '/sdcard/';
12+
export const ARTIFACTS_DIR_PREFIX = 'web-ext-artifacts-';
13+
1314
const log = createLogger(__filename);
1415

1516
export type ADBUtilsParams = {|
@@ -197,7 +198,7 @@ export default class ADBUtils {
197198
return artifactsDir;
198199
}
199200

200-
artifactsDir = `${DEVICE_DIR_BASE}${ARTIFACTS_DIR_PREFIX}-${Date.now()}`;
201+
artifactsDir = `${DEVICE_DIR_BASE}${ARTIFACTS_DIR_PREFIX}${Date.now()}`;
201202

202203
const testDirOut = (await this.runShellCommand(
203204
deviceId, `test -d ${artifactsDir} ; echo $?`
@@ -217,38 +218,38 @@ export default class ADBUtils {
217218
return artifactsDir;
218219
}
219220

220-
async checkOrCleanArtifacts(
221-
deviceId: string, remove?: boolean
221+
async detectOrRemoveOldArtifacts(
222+
deviceId: string, removeArtifactDirs?: boolean = false
222223
): Promise<boolean> {
223224
const {adbClient} = this;
224225

225-
let found = false;
226-
log.debug('Finding older artifacts');
226+
log.debug('Checking adb device for existing web-ext artifacts dirs');
227227

228228
return wrapADBCall(async () => {
229229
const files = await adbClient.readdir(deviceId, DEVICE_DIR_BASE);
230+
let found = false;
230231

231232
for (const file of files) {
232233
if (!file.isDirectory() ||
233-
!file.name.startsWith('web-ext-artifacts-')) {
234+
!file.name.startsWith(ARTIFACTS_DIR_PREFIX)) {
234235
continue;
235236
}
236237

237-
if (!remove) {
238+
// Return earlier if we only need to warn the user that some
239+
// existing artifacts dirs have been found on the adb device.
240+
if (!removeArtifactDirs) {
238241
return true;
239242
}
240243

241244
found = true;
242245

243-
const artifactsDir = `${DEVICE_DIR_BASE}/${file.name}`;
246+
const artifactsDir = `${DEVICE_DIR_BASE}${file.name}`;
244247

245248
log.debug(
246-
`Removing ${artifactsDir} artifacts directory on ${deviceId} device`
249+
`Removing artifacts directory ${artifactsDir} from device ${deviceId}`
247250
);
248251

249-
await this.runShellCommand(deviceId, [
250-
'rm', '-rf', artifactsDir,
251-
]);
252+
await this.runShellCommand(deviceId, ['rm', '-rf', artifactsDir]);
252253
}
253254

254255
return found;
@@ -269,9 +270,7 @@ export default class ADBUtils {
269270
`Removing ${artifactsDir} artifacts directory on ${deviceId} device`
270271
);
271272

272-
await this.runShellCommand(deviceId, [
273-
'rm', '-rf', artifactsDir,
274-
]);
273+
await this.runShellCommand(deviceId, ['rm', '-rf', artifactsDir]);
275274
}
276275

277276
async pushFile(

‎tests/unit/test-extension-runners/test.firefox-android.js

+18-11
Original file line numberDiff line numberDiff line change
@@ -130,9 +130,7 @@ function prepareSelectedDeviceAndAPKParams(
130130
startFirefoxAPK: sinon.spy(() => Promise.resolve()),
131131
setupForward: sinon.spy(() => Promise.resolve()),
132132
clearArtifactsDir: sinon.spy(() => Promise.resolve()),
133-
checkOrCleanArtifacts: sinon.spy(
134-
() => Promise.resolve(true)
135-
),
133+
detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(true)),
136134
setUserAbortDiscovery: sinon.spy(() => {}),
137135
ensureRequiredAPKRuntimePermissions: sinon.spy(() => Promise.resolve()),
138136
...adbOverrides,
@@ -360,11 +358,12 @@ describe('util/extension-runners/firefox-android', () => {
360358
);
361359
});
362360

363-
it('check for older artifacts', async () => {
361+
it('does check for existing artifacts dirs', async () => {
364362
const adbOverrides = {
365363
getOrCreateArtifactsDir: sinon.spy(
366364
() => Promise.resolve('/sdcard/web-ext-dir')
367365
),
366+
detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(false)),
368367
};
369368
const overriddenProperties = {
370369
params: {
@@ -373,7 +372,7 @@ describe('util/extension-runners/firefox-android', () => {
373372
buildSourceDir: sinon.spy(() => Promise.resolve({
374373
extensionPath: fakeBuiltExtensionPath,
375374
})),
376-
adbCleanArtifacts: false,
375+
adbRemoveOldArtifacts: false,
377376
},
378377
};
379378
const {
@@ -386,21 +385,26 @@ describe('util/extension-runners/firefox-android', () => {
386385
await runnerInstance.run();
387386

388387
sinon.assert.calledWithMatch(
389-
fakeADBUtils.checkOrCleanArtifacts,
388+
fakeADBUtils.detectOrRemoveOldArtifacts,
390389
'emulator-1',
391390
false,
392391
);
393392

393+
// Ensure the old artifacts are checked or removed after stopping the
394+
// apk and before creating the new artifacts dir.
394395
sinon.assert.callOrder(
395396
fakeADBUtils.amForceStopAPK,
396-
fakeADBUtils.checkOrCleanArtifacts
397+
fakeADBUtils.detectOrRemoveOldArtifacts,
398+
fakeADBUtils.getOrCreateArtifactsDir
397399
);
398400
});
399-
it('remove plausible older artifacts', async () => {
401+
402+
it('does optionally remove older artifacts dirs', async () => {
400403
const adbOverrides = {
401404
getOrCreateArtifactsDir: sinon.spy(
402405
() => Promise.resolve('/sdcard/web-ext-dir')
403406
),
407+
detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(true)),
404408
};
405409
const overriddenProperties = {
406410
params: {
@@ -409,7 +413,7 @@ describe('util/extension-runners/firefox-android', () => {
409413
buildSourceDir: sinon.spy(() => Promise.resolve({
410414
extensionPath: fakeBuiltExtensionPath,
411415
})),
412-
adbCleanArtifacts: true,
416+
adbRemoveOldArtifacts: true,
413417
},
414418
};
415419
const {
@@ -422,14 +426,17 @@ describe('util/extension-runners/firefox-android', () => {
422426
await runnerInstance.run();
423427

424428
sinon.assert.calledWithMatch(
425-
fakeADBUtils.checkOrCleanArtifacts,
429+
fakeADBUtils.detectOrRemoveOldArtifacts,
426430
'emulator-1',
427431
true,
428432
);
429433

434+
// Ensure the old artifacts are checked or removed after stopping the
435+
// apk and before creating the new artifacts dir.
430436
sinon.assert.callOrder(
431437
fakeADBUtils.amForceStopAPK,
432-
fakeADBUtils.checkOrCleanArtifacts
438+
fakeADBUtils.detectOrRemoveOldArtifacts,
439+
fakeADBUtils.getOrCreateArtifactsDir
433440
);
434441
});
435442

‎tests/unit/test-util/test.adb.js

+73-49
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,17 @@
33
import EventEmitter from 'events';
44

55
import chai from 'chai';
6-
import {describe, it} from 'mocha';
6+
import {afterEach, describe, it} from 'mocha';
77
import sinon from 'sinon';
88

99
import {
1010
UsageError,
1111
WebExtError,
1212
} from '../../../src/errors';
13-
import ADBUtils from '../../../src/util/adb';
13+
import ADBUtils, {
14+
ARTIFACTS_DIR_PREFIX,
15+
DEVICE_DIR_BASE,
16+
} from '../../../src/util/adb';
1417

1518
const fakeADBPackageList = `
1619
package:org.mozilla.fennec
@@ -623,72 +626,93 @@ describe('utils/adb', () => {
623626
});
624627
});
625628

626-
describe('checkOrCleanArtifacts', () => {
627-
const artifactDir = `web-ext-artifacts-${Date.now()}`;
628-
function makeFakeArtifact(artifactName, isDirectory) {
629+
describe('detectOrRemoveOldArtifacts', () => {
630+
function createFakeReaddirFile(artifactName: string, isDirectory: boolean) {
629631
return {
630632
name: artifactName,
631633
isDirectory: () => {
632634
return isDirectory;
633635
},
634636
};
635637
}
636-
const files = [
637-
makeFakeArtifact(artifactDir.concat('fake-dir1'), true),
638-
makeFakeArtifact(artifactDir.concat('fake-dir2'), true),
639-
makeFakeArtifact(artifactDir.concat('fake-file1'), false),
640-
makeFakeArtifact('not-web-ext-artifacts-dir', true),
638+
639+
const filesNotArtifactsDirs = [
640+
createFakeReaddirFile('not-an-artifact-dir1', true),
641+
createFakeReaddirFile('not-a-dir2', false),
641642
];
642643

643-
it('checks old artifacts', async () => {
644-
const adb = getFakeADBKit({
645-
adbClient: {
646-
readdir: sinon.spy(() => Promise.resolve(files)),
647-
shell: sinon.spy(() => Promise.resolve('')),
648-
},
649-
adbkitUtil: {
650-
readAll: sinon.spy(() => Promise.resolve(Buffer.from('1\n'))),
651-
},
652-
});
644+
const filesArtifactsDirs = [
645+
createFakeReaddirFile(`${ARTIFACTS_DIR_PREFIX}1`, true),
646+
createFakeReaddirFile(`${ARTIFACTS_DIR_PREFIX}2`, true),
647+
];
648+
649+
const allFiles = [...filesNotArtifactsDirs, ...filesArtifactsDirs];
650+
651+
const sb = sinon.createSandbox();
652+
const adbkitSpies = {
653+
adbClient: {
654+
readdir: sb.spy(() => Promise.resolve([])),
655+
shell: sb.spy(() => Promise.resolve('')),
656+
},
657+
adbkitUtil: {
658+
readAll: sb.spy(() => Promise.resolve(Buffer.from('1\n'))),
659+
},
660+
};
661+
662+
// Reset the fakeADBClient spies after each test case.
663+
afterEach(() => sb.reset());
664+
665+
it('does detect old artifacts directories', async () => {
666+
const adb = getFakeADBKit(adbkitSpies);
653667
const adbUtils = new ADBUtils({adb});
668+
const fakeADB = adb.fakeADBClient;
654669

655-
const promise = adbUtils.checkOrCleanArtifacts('device1', false);
656-
const result = await assert.isFulfilled(promise);
657-
assert.equal(result, true);
670+
fakeADB.readdir = sb.spy(async () => filesNotArtifactsDirs);
658671

659-
sinon.assert.calledOnce(adb.fakeADBClient.readdir);
660-
//While checking of files shell shoudln't be called
661-
sinon.assert.notCalled(adb.fakeADBClient.shell);
672+
await assert.becomes(
673+
adbUtils.detectOrRemoveOldArtifacts('device1', false),
674+
false,
675+
'Expected to return false when no old artifacts dirs have been found'
676+
);
677+
sinon.assert.calledOnce(fakeADB.readdir);
678+
sinon.assert.calledWith(fakeADB.readdir, 'device1', DEVICE_DIR_BASE);
679+
// Expect adbkit shell to never be called when no artifacts have been found.
680+
sinon.assert.notCalled(fakeADB.shell);
681+
682+
adb.fakeADBClient.readdir = sb.spy(async () => allFiles);
683+
684+
await assert.becomes(
685+
adbUtils.detectOrRemoveOldArtifacts('device1', false),
686+
true,
687+
'Expected to return true when old artifacts dirs have been found'
688+
);
689+
sinon.assert.notCalled(fakeADB.shell);
662690
});
663691

664-
it('removes plausible artifacts directory', async () => {
665-
const adb = getFakeADBKit({
666-
adbClient: {
667-
readdir: sinon.spy(() => Promise.resolve(files)),
668-
shell: sinon.spy(() => Promise.resolve('')),
669-
},
670-
adbkitUtil: {
671-
readAll: sinon.spy(() => Promise.resolve(Buffer.from('1\n'))),
672-
},
673-
});
692+
it('does optionally remove artifacts directories', async () => {
693+
const adb = getFakeADBKit(adbkitSpies);
674694
const adbUtils = new ADBUtils({adb});
675-
const artifactDirFullPath1 = `/sdcard/${artifactDir}fake-dir1`;
676-
const artifactDirFullPath2 = `/sdcard/${artifactDir}fake-dir2`;
677695

678-
const promise = adbUtils.checkOrCleanArtifacts('device1', true);
679-
const result = await assert.isFulfilled(promise);
680-
assert.equal(result, true);
696+
adb.fakeADBClient.readdir = sb.spy(async () => allFiles);
681697

682-
sinon.assert.calledOnce(adb.fakeADBClient.readdir);
683-
sinon.assert.calledTwice(adb.fakeADBClient.shell);
684-
sinon.assert.calledWithMatch(
685-
adb.fakeADBClient.shell, 'device1',
686-
['rm', '-rf', artifactDirFullPath1]
698+
await assert.becomes(
699+
adbUtils.detectOrRemoveOldArtifacts('device1', true),
700+
true,
701+
'Expected to return true when old artifacts dirs have been found'
687702
);
688-
sinon.assert.calledWithMatch(
689-
adb.fakeADBClient.shell, 'device1',
690-
['rm', '-rf', artifactDirFullPath2]
703+
704+
sinon.assert.calledOnce(adb.fakeADBClient.readdir);
705+
assert.equal(
706+
adb.fakeADBClient.shell.callCount,
707+
filesArtifactsDirs.length,
691708
);
709+
710+
for (const fakeFile of filesArtifactsDirs) {
711+
sinon.assert.calledWithMatch(
712+
adb.fakeADBClient.shell, 'device1',
713+
['rm', '-rf', `${DEVICE_DIR_BASE}${fakeFile.name}`]
714+
);
715+
}
692716
});
693717
});
694718

0 commit comments

Comments
 (0)
Please sign in to comment.