test(@angular/cli): improve environment variables clean up during E2E's

(cherry picked from commit 99bd478)
angular · Jul 21, 2021 · ec79ab1 · ec79ab1
1 parent eaa2378
commit ec79ab1
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 101 deletions.
diff --git a/tests/legacy-cli/e2e/tests/commands/add/npm-env-vars.ts b/tests/legacy-cli/e2e/tests/commands/add/npm-env-vars.ts
@@ -1,33 +1,26 @@
 import { expectFileNotToExist, expectFileToExist } from '../../../utils/fs';
-import { getActivePackageManager } from '../../../utils/packages';
 import { git, ng } from '../../../utils/process';
 import {
   createNpmConfigForAuthentication,
   setNpmEnvVarsForAuthentication,
 } from '../../../utils/registry';
 
 export default async function () {
-  const packageManager = getActivePackageManager();
+  // Yarn also supports NPM_CONFIG env variables.
+  // https://classic.yarnpkg.com/en/docs/envvars/
 
-  if (packageManager === 'npm') {
-    const originalEnvironment = { ...process.env };
-    try {
-      const command = ['add', '@angular/pwa', '--skip-confirmation'];
+  const command = ['add', '@angular/pwa', '--skip-confirmation'];
 
-      // Environment variables only
-      await expectFileNotToExist('src/manifest.webmanifest');
-      setNpmEnvVarsForAuthentication();
-      await ng(...command);
-      await expectFileToExist('src/manifest.webmanifest');
-      await git('clean', '-dxf');
+  // Environment variables only
+  await expectFileNotToExist('src/manifest.webmanifest');
+  setNpmEnvVarsForAuthentication();
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+  await git('clean', '-dxf');
 
-      // Mix of config file and env vars works
-      await expectFileNotToExist('src/manifest.webmanifest');
-      await createNpmConfigForAuthentication(false, true);
-      await ng(...command);
-      await expectFileToExist('src/manifest.webmanifest');
-    } finally {
-      process.env = originalEnvironment;
-    }
-  }
+  // Mix of config file and env vars works
+  await expectFileNotToExist('src/manifest.webmanifest');
+  await createNpmConfigForAuthentication(false, true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
 }
diff --git a/tests/legacy-cli/e2e/tests/commands/add/registry-option.ts b/tests/legacy-cli/e2e/tests/commands/add/registry-option.ts
@@ -10,21 +10,12 @@ export default async function () {
   await writeMultipleFiles({
     '.npmrc': 'registry=http://127.0.0.1:9999',
   });
+
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
+  delete process.env['NPM_CONFIG_REGISTRY'];
 
-  try {
-    await expectToFail(() => ng('add', '@angular/pwa', '--skip-confirmation'));
+  await expectToFail(() => ng('add', '@angular/pwa', '--skip-confirmation'));
 
-    await ng('add', `--registry=${testRegistry}`, '@angular/pwa', '--skip-confirmation');
-    await expectFileToExist('src/manifest.webmanifest');
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await ng('add', `--registry=${testRegistry}`, '@angular/pwa', '--skip-confirmation');
+  await expectFileToExist('src/manifest.webmanifest');
 }
diff --git a/tests/legacy-cli/e2e/tests/commands/add/secure-registry.ts b/tests/legacy-cli/e2e/tests/commands/add/secure-registry.ts
@@ -5,38 +5,28 @@ import { expectToFail } from '../../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
+  delete process.env['NPM_CONFIG_REGISTRY'];
 
-  try {
-    const command = ['add', '@angular/pwa', '--skip-confirmation'];
-    await expectFileNotToExist('src/manifest.webmanifest');
+  const command = ['add', '@angular/pwa', '--skip-confirmation'];
+  await expectFileNotToExist('src/manifest.webmanifest');
 
-    // Works with unscoped registry authentication details
-    await createNpmConfigForAuthentication(false);
-    await ng(...command);
-    await expectFileToExist('src/manifest.webmanifest');
-    await git('clean', '-dxf');
+  // Works with unscoped registry authentication details
+  await createNpmConfigForAuthentication(false);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
+  await git('clean', '-dxf');
 
-    // Works with scoped registry authentication details
-    await expectFileNotToExist('src/manifest.webmanifest');
+  // Works with scoped registry authentication details
+  await expectFileNotToExist('src/manifest.webmanifest');
 
-    await createNpmConfigForAuthentication(true);
-    await ng(...command);
-    await expectFileToExist('src/manifest.webmanifest');
+  await createNpmConfigForAuthentication(true);
+  await ng(...command);
+  await expectFileToExist('src/manifest.webmanifest');
 
-    // Invalid authentication token
-    await createNpmConfigForAuthentication(false, true);
-    await expectToFail(() => ng(...command));
+  // Invalid authentication token
+  await createNpmConfigForAuthentication(false, true);
+  await expectToFail(() => ng(...command));
 
-    await createNpmConfigForAuthentication(true, true);
-    await expectToFail(() => ng(...command));
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await createNpmConfigForAuthentication(true, true);
+  await expectToFail(() => ng(...command));
 }
diff --git a/tests/legacy-cli/e2e/tests/update/update-secure-registry.ts b/tests/legacy-cli/e2e/tests/update/update-secure-registry.ts
@@ -4,37 +4,26 @@ import { expectToFail } from '../../utils/utils';
 
 export default async function () {
   // The environment variable has priority over the .npmrc
-  let originalRegistryVariable;
-  if (process.env['NPM_CONFIG_REGISTRY']) {
-    originalRegistryVariable = process.env['NPM_CONFIG_REGISTRY'];
-    delete process.env['NPM_CONFIG_REGISTRY'];
-  }
-
+  delete process.env['NPM_CONFIG_REGISTRY'];
   const worksMessage = 'We analyzed your package.json';
 
-  try {
-    // Valid authentication token
-    await createNpmConfigForAuthentication(false);
-    const { stdout: stdout1 } = await ng('update');
-    if (!stdout1.includes(worksMessage)) {
-      throw new Error(`Expected stdout to contain "${worksMessage}"`);
-    }
+  // Valid authentication token
+  await createNpmConfigForAuthentication(false);
+  const { stdout: stdout1 } = await ng('update');
+  if (!stdout1.includes(worksMessage)) {
+    throw new Error(`Expected stdout to contain "${worksMessage}"`);
+  }
 
-    await createNpmConfigForAuthentication(true);
-    const { stdout: stdout2 } = await ng('update');
-    if (!stdout2.includes(worksMessage)) {
-      throw new Error(`Expected stdout to contain "${worksMessage}"`);
-    }
+  await createNpmConfigForAuthentication(true);
+  const { stdout: stdout2 } = await ng('update');
+  if (!stdout2.includes(worksMessage)) {
+    throw new Error(`Expected stdout to contain "${worksMessage}"`);
+  }
 
-    // Invalid authentication token
-    await createNpmConfigForAuthentication(false, true);
-    await expectToFail(() => ng('update'));
+  // Invalid authentication token
+  await createNpmConfigForAuthentication(false, true);
+  await expectToFail(() => ng('update'));
 
-    await createNpmConfigForAuthentication(true, true);
-    await expectToFail(() => ng('update'));
-  } finally {
-    if (originalRegistryVariable) {
-      process.env['NPM_CONFIG_REGISTRY'] = originalRegistryVariable;
-    }
-  }
+  await createNpmConfigForAuthentication(true, true);
+  await expectToFail(() => ng('update'));
 }
diff --git a/tests/legacy-cli/e2e/utils/packages.ts b/tests/legacy-cli/e2e/utils/packages.ts
@@ -53,8 +53,7 @@ export async function setRegistry(useTestRegistry: boolean): Promise<void> {
     // Safe to set a user configuration on CI
     await npm('config', 'set', 'registry', url);
   } else {
-    // Yarn does not use the environment variable so an .npmrc file is also required
-    await writeFile('.npmrc', `registry=${url}`);
+    // Yarn supports both `NPM_CONFIG_REGISTRY` and `YARN_REGISTRY`.
     process.env['NPM_CONFIG_REGISTRY'] = url;
   }
 }
diff --git a/tests/legacy-cli/e2e_runner.ts b/tests/legacy-cli/e2e_runner.ts
@@ -173,16 +173,19 @@ testsToRun
         .then(() => {
           // If we're not in a setup, change the directory back to where it was before the test.
           // This allows tests to chdir without worrying about keeping the original directory.
-          if (allSetups.indexOf(relativeName) == -1 && previousDir) {
+          if (!allSetups.includes(relativeName) && previousDir) {
             process.chdir(previousDir);
+
+            // Restore env variables before each test.
+            console.log('  Restoring original environment variables...');
+            process.env = originalEnvVariables;
           }
         })
         .then(() => {
           // Only clean after a real test, not a setup step. Also skip cleaning if the test
           // requested an exception.
-          if (allSetups.indexOf(relativeName) == -1 && clean) {
+          if (!allSetups.includes(relativeName) && clean) {
             logStack.push(new logging.NullLogger());
-
             return gitClean().then(
               () => logStack.pop(),
               (err) => {
@@ -192,11 +195,6 @@ testsToRun
             );
           }
         })
-        .finally(() => {
-          // Restore env variables after each test.
-          console.log('  Restoring original environment variables...');
-          process.env = originalEnvVariables;
-        })
         .then(
           () => printFooter(currentFileName, start),
           (err) => {