chore: Renamed cli option to --adb-remove-old-artifacts and some mino…

…r tweaks to the related tests
mozilla · Jul 20, 2020 · 011dffe · 011dffe
1 parent a2e4731
commit 011dffe
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 109 deletions.
diff --git a/src/cmd/run.js b/src/cmd/run.js
@@ -45,7 +45,7 @@ export type CmdRunParams = {|
   adbPort?: string,
   adbDevice?: string,
   adbDiscoveryTimeout?: number,
-  adbCleanArtifacts?: boolean,
+  adbRemoveOldArtifacts?: boolean,
   firefoxApk?: string,
   firefoxApkComponent?: string,
 
@@ -88,7 +88,7 @@ export default async function run(
     adbPort,
     adbDevice,
     adbDiscoveryTimeout,
-    adbCleanArtifacts,
+    adbRemoveOldArtifacts,
     firefoxApk,
     firefoxApkComponent,
     // Chromium CLI options.
@@ -167,7 +167,7 @@ export default async function run(
       adbPort,
       adbBin,
       adbDiscoveryTimeout,
-      adbCleanArtifacts,
+      adbRemoveOldArtifacts,
 
       // Injected dependencies.
       firefoxApp,

diff --git a/src/extension-runners/firefox-android.js b/src/extension-runners/firefox-android.js
@@ -71,7 +71,7 @@ export type FirefoxAndroidExtensionRunnerParams = {|
   adbPort?: string,
   adbDevice?: string,
   adbDiscoveryTimeout?: number,
-  adbCleanArtifacts?: boolean,
+  adbRemoveOldArtifacts?: boolean,
   firefoxApk?: string,
   firefoxApkComponent?: string,
 
@@ -385,30 +385,6 @@ export class FirefoxAndroidExtensionRunner {
     await adbUtils.amForceStopAPK(selectedAdbDevice, selectedFirefoxApk);
   }
 
-  async adbOldArtifactsDir(removeArtifacts?: boolean) {
-    const {
-      adbUtils,
-      selectedAdbDevice,
-    } = this;
-
-    const foundDirectories = await adbUtils.checkOrCleanArtifacts(
-      selectedAdbDevice, removeArtifacts
-    );
-
-    if (!foundDirectories) {
-      return;
-    }
-    if (removeArtifacts) {
-      log.info('Old web-ext artifacts have been found and removed ' +
-              `from ${selectedAdbDevice} device`);
-    } else {
-      log.warn(
-        `Old artifacts directories have been found on ${selectedAdbDevice} ` +
-        'device. Use --adb-clean-artifacts to remove them automatically.'
-      );
-    }
-  }
-
   async adbCheckRuntimePermissions() {
     const {
       adbUtils,
@@ -456,6 +432,7 @@ export class FirefoxAndroidExtensionRunner {
       params: {
         customPrefs,
         firefoxApp,
+        adbRemoveOldArtifacts,
       },
     } = this;
     // Create the preferences file and the Fennec temporary profile.
@@ -466,8 +443,23 @@ export class FirefoxAndroidExtensionRunner {
       customPrefs,
     });
 
-    //Checking for older artifacts
-    await this.adbOldArtifactsDir(this.params.adbCleanArtifacts);
+    // Check if there are any artifacts dirs from previous runs and
+    // automatically remove them if adbRemoteOldArtifacts is true.
+    const foundOldArtifacts = await adbUtils.detectOrRemoveOldArtifacts(
+      selectedAdbDevice, adbRemoveOldArtifacts
+    );
+
+    if (foundOldArtifacts) {
+      if (adbRemoveOldArtifacts) {
+        log.info('Old web-ext artifacts have been found and removed ' +
+          `from ${selectedAdbDevice} device`);
+      } else {
+        log.warn(
+          `Old artifacts directories have been found on ${selectedAdbDevice} ` +
+          'device. Use --adb-remove-old-artifacts to remove them automatically.'
+        );
+      }
+    }
 
     // Choose a artifacts dir name for the assets pushed to the
     // Android device.

diff --git a/src/program.js b/src/program.js
@@ -663,8 +663,8 @@ Example: $0 --help run.
         type: 'number',
         requiresArg: true,
       },
-      'adb-clean-artifacts': {
-        describe: 'Remove old artifact directories from the adb device',
+      'adb-remove-old-artifacts': {
+        describe: 'Remove old artifacts directories from the adb device',
         demandOption: false,
         type: 'boolean',
       },

diff --git a/src/util/adb.js b/src/util/adb.js
@@ -8,8 +8,9 @@ import {
 } from '../errors';
 import {createLogger} from '../util/logger';
 
-const DEVICE_DIR_BASE = '/sdcard';
-const ARTIFACTS_DIR_PREFIX = '/web-ext-artifacts';
+export const DEVICE_DIR_BASE = '/sdcard/';
+export const ARTIFACTS_DIR_PREFIX = 'web-ext-artifacts-';
+
 const log = createLogger(__filename);
 
 export type ADBUtilsParams = {|
@@ -197,7 +198,7 @@ export default class ADBUtils {
       return artifactsDir;
     }
 
-    artifactsDir = `${DEVICE_DIR_BASE}${ARTIFACTS_DIR_PREFIX}-${Date.now()}`;
+    artifactsDir = `${DEVICE_DIR_BASE}${ARTIFACTS_DIR_PREFIX}${Date.now()}`;
 
     const testDirOut = (await this.runShellCommand(
       deviceId, `test -d ${artifactsDir} ; echo $?`
@@ -217,38 +218,38 @@ export default class ADBUtils {
     return artifactsDir;
   }
 
-  async checkOrCleanArtifacts(
-    deviceId: string, remove?: boolean
+  async detectOrRemoveOldArtifacts(
+    deviceId: string, removeArtifactDirs?: boolean = false
   ): Promise<boolean> {
     const {adbClient} = this;
 
-    let found = false;
-    log.debug('Finding older artifacts');
+    log.debug('Checking adb device for existing web-ext artifacts dirs');
 
     return wrapADBCall(async () => {
       const files = await adbClient.readdir(deviceId, DEVICE_DIR_BASE);
+      let found = false;
 
       for (const file of files) {
         if (!file.isDirectory() ||
-              !file.name.startsWith('web-ext-artifacts-')) {
+            !file.name.startsWith(ARTIFACTS_DIR_PREFIX)) {
           continue;
         }
 
-        if (!remove) {
+        // Return earlier if we only need to warn the user that some
+        // existing artifacts dirs have been found on the adb device.
+        if (!removeArtifactDirs) {
           return true;
         }
 
         found = true;
 
-        const artifactsDir = `${DEVICE_DIR_BASE}/${file.name}`;
+        const artifactsDir = `${DEVICE_DIR_BASE}${file.name}`;
 
         log.debug(
-          `Removing ${artifactsDir} artifacts directory on ${deviceId} device`
+          `Removing artifacts directory ${artifactsDir} from device ${deviceId}`
         );
 
-        await this.runShellCommand(deviceId, [
-          'rm', '-rf', artifactsDir,
-        ]);
+        await this.runShellCommand(deviceId, ['rm', '-rf', artifactsDir]);
       }
 
       return found;
@@ -269,9 +270,7 @@ export default class ADBUtils {
       `Removing ${artifactsDir} artifacts directory on ${deviceId} device`
     );
 
-    await this.runShellCommand(deviceId, [
-      'rm', '-rf', artifactsDir,
-    ]);
+    await this.runShellCommand(deviceId, ['rm', '-rf', artifactsDir]);
   }
 
   async pushFile(

diff --git a/tests/unit/test-extension-runners/test.firefox-android.js b/tests/unit/test-extension-runners/test.firefox-android.js
@@ -130,9 +130,7 @@ function prepareSelectedDeviceAndAPKParams(
     startFirefoxAPK: sinon.spy(() => Promise.resolve()),
     setupForward: sinon.spy(() => Promise.resolve()),
     clearArtifactsDir: sinon.spy(() => Promise.resolve()),
-    checkOrCleanArtifacts: sinon.spy(
-      () => Promise.resolve(true)
-    ),
+    detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(true)),
     setUserAbortDiscovery: sinon.spy(() => {}),
     ensureRequiredAPKRuntimePermissions: sinon.spy(() => Promise.resolve()),
     ...adbOverrides,
@@ -360,11 +358,12 @@ describe('util/extension-runners/firefox-android', () => {
          );
        });
 
-    it('check for older artifacts', async () => {
+    it('does check for existing artifacts dirs', async () => {
       const adbOverrides = {
         getOrCreateArtifactsDir: sinon.spy(
           () => Promise.resolve('/sdcard/web-ext-dir')
         ),
+        detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(false)),
       };
       const overriddenProperties = {
         params: {
@@ -373,7 +372,7 @@ describe('util/extension-runners/firefox-android', () => {
           buildSourceDir: sinon.spy(() => Promise.resolve({
             extensionPath: fakeBuiltExtensionPath,
           })),
-          adbCleanArtifacts: false,
+          adbRemoveOldArtifacts: false,
         },
       };
       const {
@@ -386,21 +385,26 @@ describe('util/extension-runners/firefox-android', () => {
       await runnerInstance.run();
 
       sinon.assert.calledWithMatch(
-        fakeADBUtils.checkOrCleanArtifacts,
+        fakeADBUtils.detectOrRemoveOldArtifacts,
         'emulator-1',
         false,
       );
 
+      // Ensure the old artifacts are checked or removed after stopping the
+      // apk and before creating the new artifacts dir.
       sinon.assert.callOrder(
         fakeADBUtils.amForceStopAPK,
-        fakeADBUtils.checkOrCleanArtifacts
+        fakeADBUtils.detectOrRemoveOldArtifacts,
+        fakeADBUtils.getOrCreateArtifactsDir
       );
     });
-    it('remove plausible older artifacts', async () => {
+
+    it('does optionally remove older artifacts dirs', async () => {
       const adbOverrides = {
         getOrCreateArtifactsDir: sinon.spy(
           () => Promise.resolve('/sdcard/web-ext-dir')
         ),
+        detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(true)),
       };
       const overriddenProperties = {
         params: {
@@ -409,7 +413,7 @@ describe('util/extension-runners/firefox-android', () => {
           buildSourceDir: sinon.spy(() => Promise.resolve({
             extensionPath: fakeBuiltExtensionPath,
           })),
-          adbCleanArtifacts: true,
+          adbRemoveOldArtifacts: true,
         },
       };
       const {
@@ -422,14 +426,17 @@ describe('util/extension-runners/firefox-android', () => {
       await runnerInstance.run();
 
       sinon.assert.calledWithMatch(
-        fakeADBUtils.checkOrCleanArtifacts,
+        fakeADBUtils.detectOrRemoveOldArtifacts,
         'emulator-1',
         true,
       );
 
+      // Ensure the old artifacts are checked or removed after stopping the
+      // apk and before creating the new artifacts dir.
       sinon.assert.callOrder(
         fakeADBUtils.amForceStopAPK,
-        fakeADBUtils.checkOrCleanArtifacts
+        fakeADBUtils.detectOrRemoveOldArtifacts,
+        fakeADBUtils.getOrCreateArtifactsDir
       );
     });