unit tests and handling of 'false' string

iankhou · iankhou · commit e386e4fd9a15 · 2025-08-28T15:31:03.000-04:00
diff --git a/packages/aws-cdk/lib/cli/cli.ts b/packages/aws-cdk/lib/cli/cli.ts
@@ -121,7 +121,23 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise<n
    * systems, even though technically we maybe could.
    */
   const isSafeToWriteNotices = !isCI() || Boolean(ciSystemIsStdErrSafe());
-  const shouldDisplayNotices = isSafeToWriteNotices && (configuration.settings.get(['notices']) !== false);
+  
+  // Determine if notices should be displayed based on CLI args and configuration
+  let shouldDisplayNotices: boolean;
+  if (argv.notices !== undefined) {
+    // CLI argument takes precedence
+    shouldDisplayNotices = argv.notices;
+  } else {
+    // Fall back to configuration file setting, then autodetection
+    const configNotices = configuration.settings.get(['notices']);
+    if (configNotices !== undefined) {
+      // Consider string "false" to be falsy in this context
+      shouldDisplayNotices = configNotices !== "false" && Boolean(configNotices);
+    } else {
+      // Default autodetection behavior
+      shouldDisplayNotices = isSafeToWriteNotices;
+    }
+  }
 
   // Notices either go to stderr, or nowhere
   ioHost.noticesDestination = shouldDisplayNotices ? 'stderr' : 'drop';
diff --git a/packages/aws-cdk/test/cli/cli.test.ts b/packages/aws-cdk/test/cli/cli.test.ts
@@ -2,6 +2,7 @@ import { exec } from '../../lib/cli/cli';
 import { CliIoHost } from '../../lib/cli/io-host';
 import { Configuration } from '../../lib/cli/user-configuration';
 import { TestIoHost } from '../_helpers/io-host';
+import { Notices } from '../../lib/api/notices';
 
 // Store original version module exports so we don't conflict with other tests
 const originalVersion = jest.requireActual('../../lib/cli/version');
@@ -30,24 +31,26 @@ const actualUserConfig = jest.requireActual('../../lib/cli/user-configuration');
 Configuration.fromArgs = jest.fn().mockImplementation(() => actualUserConfig.Configuration.fromArgs(ioHelper));
 Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => actualUserConfig.Configuration.fromArgs(ioHelper));
 
-jest.mock('../../lib/api/notices', () => ({
-  Notices: {
-    create: jest.fn().mockReturnValue({
-      refresh: jest.fn().mockResolvedValue(undefined),
-      display: jest.fn(),
-    }),
-  },
-}));
-
 jest.mock('../../lib/cli/parse-command-line-arguments', () => ({
-  parseCommandLineArguments: jest.fn().mockImplementation((args) => Promise.resolve({
-    _: ['version'],
-    verbose: args.includes('-v') ? (
-      args.filter((arg: string) => arg === '-v').length
-    ) : args.includes('--verbose') ? (
-      parseInt(args[args.indexOf('--verbose') + 1]) || true
-    ) : undefined,
-  })),
+  parseCommandLineArguments: jest.fn().mockImplementation((args) => {
+    const result: any = {
+      _: ['version'],
+      verbose: args.includes('-v') ? (
+        args.filter((arg: string) => arg === '-v').length
+      ) : args.includes('--verbose') ? (
+        parseInt(args[args.indexOf('--verbose') + 1]) || true
+      ) : undefined,
+    };
+
+    // Handle notices flags
+    if (args.includes('--notices')) {
+      result.notices = true;
+    } else if (args.includes('--no-notices')) {
+      result.notices = false;
+    }
+
+    return Promise.resolve(result);
+  }),
 }));
 
 describe('exec verbose flag tests', () => {
@@ -97,3 +100,296 @@ describe('exec verbose flag tests', () => {
     expect(CliIoHost.instance().logLevel).toBe('trace');
   });
 });
+
+describe('notices configuration tests', () => {
+  let mockNoticesCreate: jest.SpyInstance;
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    
+    // Mock the Notices.create method
+    mockNoticesCreate = jest.spyOn(Notices, 'create').mockReturnValue({
+      refresh: jest.fn().mockResolvedValue(undefined),
+      display: jest.fn(),
+    } as any);
+    
+    // Set up version module for our tests
+    jest.mock('../../lib/cli/version', () => ({
+      ...originalVersion,
+      DISPLAY_VERSION: 'test-version',
+      displayVersionMessage: jest.fn().mockResolvedValue(undefined),
+    }));
+  });
+
+  afterEach(() => {
+    mockNoticesCreate.mockRestore();
+    // Restore the version module to its original state
+    jest.resetModules();
+    jest.setMock('../../lib/cli/version', originalVersion);
+  });
+
+  test('should send notices to "stderr" when passing --notices flag in CLI', async () => {
+    await exec(['--notices', 'version']);
+    
+    expect(mockNoticesCreate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        "ioHost": expect.objectContaining({
+          "noticesDestination": "stderr",
+        }),
+      })
+    );
+  });
+
+  test('should send notices to "drop" when passing --no-notices in CLI', async () => {
+    await exec(['--no-notices', 'version']);
+    
+    expect(mockNoticesCreate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        "ioHost": expect.objectContaining({
+          "noticesDestination": "drop",
+        }),
+      })
+    );
+  });
+
+    test('should send notices to "drop" when notices: false in settings and no CLI flag is provided', async () => {
+      // Mock configuration to return notices: false
+      const mockConfig = {
+        loadConfigFiles: jest.fn().mockResolvedValue(undefined),
+        settings: {
+          get: jest.fn().mockImplementation((key: string[]) => {
+            if (key[0] === 'notices') return false;
+            return undefined;
+          }),
+        },
+        context: {
+          get: jest.fn().mockReturnValue([]),
+        },
+      };
+
+      (Configuration as any).mockImplementation(() => mockConfig);
+      Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig);
+
+      await exec(['version']);
+
+      expect(mockNoticesCreate).toHaveBeenCalledWith(
+        expect.objectContaining({
+          "ioHost": expect.objectContaining({
+            "noticesDestination": "drop",
+          }),
+        })
+      );
+    });
+
+    test.each([
+      { 
+        envVar: 'TEAMCITY_VERSION', 
+        description: 'TeamCity',
+        scenarios: [
+          { 
+            name: 'should send notices to "drop" by default when no settings or CLI flags are provided',
+            configNotices: undefined,
+            cliArgs: ['version'],
+            expectedDestination: 'drop'
+          },
+          {
+            name: 'should send notices to "stderr" when config setting notices=true',
+            configNotices: true,
+            cliArgs: ['version'],
+            expectedDestination: 'stderr'
+          },
+          {
+            name: 'should send notices to "stderr" when passing --notices CLI flag',
+            configNotices: undefined,
+            cliArgs: ['--notices', 'version'],
+            expectedDestination: 'stderr'
+          },
+          {
+            name: 'should send notices to "drop"  when passing --no-notices CLI flag, even when config has notices=true',
+            configNotices: true,
+            cliArgs: ['--no-notices', 'version'],
+            expectedDestination: 'drop'
+          }
+        ]
+      },
+      { 
+        envVar: 'TF_BUILD', 
+        description: 'Azure DevOps',
+        scenarios: [
+          { 
+            name: 'should send notices to "drop" when no settings or CLI flags are provided',
+            configNotices: undefined,
+            cliArgs: ['version'],
+            expectedDestination: 'drop'
+          },
+          {
+            name: 'should send notices to "stderr" config setting notices=true',
+            configNotices: true,
+            cliArgs: ['version'],
+            expectedDestination: 'stderr'
+          },
+          {
+            name: 'should send notices to "stderr" --notices CLI flag',
+            configNotices: undefined,
+            cliArgs: ['--notices', 'version'],
+            expectedDestination: 'stderr'
+          },
+          {
+            name: 'should send notices to "drop" when passing --no-notices CLI flag, even when config has notices=true',
+            configNotices: true,
+            cliArgs: ['--no-notices', 'version'],
+            expectedDestination: 'drop'
+          }
+        ]
+      },
+    ])('CI environment with $description', async ({ envVar, scenarios }) => {
+      for (const scenario of scenarios) {
+        // Store original environment variables
+        const originalCI = process.env.CI;
+        const originalEnvVar = process.env[envVar];
+
+        // Set CI environment variables
+        process.env.CI = '1';
+        process.env[envVar] = '1';
+
+        try {
+          // Mock configuration
+          const mockConfig = {
+            loadConfigFiles: jest.fn().mockResolvedValue(undefined),
+            settings: {
+              get: jest.fn().mockImplementation((key: string[]) => {
+                if (key[0] === 'notices') return scenario.configNotices;
+                return undefined;
+              }),
+            },
+            context: {
+              get: jest.fn().mockReturnValue([]),
+            },
+          };
+
+          (Configuration as any).mockImplementation(() => mockConfig);
+          Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig);
+
+          await exec(scenario.cliArgs);
+
+          expect(mockNoticesCreate).toHaveBeenCalledWith(
+            expect.objectContaining({
+              "ioHost": expect.objectContaining({
+                "noticesDestination": scenario.expectedDestination,
+              }),
+            })
+          );
+        } finally {
+          // Restore original environment variables
+          if (originalCI !== undefined) {
+            process.env.CI = originalCI;
+          } else {
+            delete process.env.CI;
+          }
+          if (originalEnvVar !== undefined) {
+            process.env[envVar] = originalEnvVar;
+          } else {
+            delete process.env[envVar];
+          }
+        }
+      }
+    });
+
+  test('should read notices=true setting from configuration', async () => {
+    // Mock configuration to return notices: true
+    const mockConfig = {
+      loadConfigFiles: jest.fn().mockResolvedValue(undefined),
+      settings: {
+        get: jest.fn().mockImplementation((key: string) => {
+          if (key[0] === 'notices') return true;
+          return undefined;
+        }),
+      },
+      context: {
+        get: jest.fn().mockReturnValue([]),
+      },
+    };
+    
+    (Configuration as any).mockImplementation(() => mockConfig);
+    Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig);
+
+    await exec(['version']);
+    
+    expect(mockNoticesCreate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        "ioHost": expect.objectContaining({
+          "noticesDestination": "stderr",
+        }),
+      })
+    );
+  });
+
+  test('should send notices to "drop" when passing --no-notices in CLI and config set to notices: false', async () => {
+    // Mock configuration to return notices: true, but CLI flag should override
+    const mockConfig = {
+      loadConfigFiles: jest.fn().mockResolvedValue(undefined),
+      settings: {
+        get: jest.fn().mockImplementation((key: string) => {
+          if (key[0] === 'notices') return true;
+          return undefined;
+        }),
+      },
+      context: {
+        get: jest.fn().mockReturnValue([]),
+      },
+    };
+    
+    (Configuration as any).mockImplementation(() => mockConfig);
+    Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig);
+
+    await exec(['--no-notices', 'version']);
+    
+    expect(mockNoticesCreate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        "ioHost": expect.objectContaining({
+          "noticesDestination": "drop",
+        }),
+      })
+    );
+  });
+
+  test.each([
+    { value: undefined, expected: 'stderr', description: 'undefined (autodetection)' },
+    { value: false, expected: 'drop', description: 'boolean false' },
+    { value: true, expected: 'stderr', description: 'boolean true' },
+    // support string "false" as false
+    { value: 'false', expected: 'drop', description: 'string "false"' },
+    { value: 'truthy', expected: 'stderr', description: 'string "truthy"' },
+    { value: 0, expected: 'drop', description: 'numeric 0' },
+    { value: 1, expected: 'stderr', description: 'numeric 1' },
+    { value: '', expected: 'drop', description: 'empty string' },
+    { value: null, expected: 'drop', description: 'null' },
+  ])('should send notices to "$expected" config value: $description', async ({ value, expected }) => {
+    // Mock configuration to return the test value
+    const mockConfig = {
+      loadConfigFiles: jest.fn().mockResolvedValue(undefined),
+      settings: {
+        get: jest.fn().mockImplementation((key: string[]) => {
+          if (key[0] === 'notices') return value;
+          return undefined;
+        }),
+      },
+      context: {
+        get: jest.fn().mockReturnValue([]),
+      },
+    };
+    
+    (Configuration as any).mockImplementation(() => mockConfig);
+    Configuration.fromArgsAndFiles = jest.fn().mockImplementation(() => mockConfig);
+
+    await exec(['version']);
+    
+    expect(mockNoticesCreate).toHaveBeenCalledWith(
+      expect.objectContaining({
+        "ioHost": expect.objectContaining({
+          "noticesDestination": expected,
+        }),
+      })
+    );
+  });
+});
