fix: merge apps by appId, update runtime config tests, add mergeSiteConfig unit tests

[brian-smith-tcril]

Summary

• Update runtime config tests to match the new implementation that removes siteId and simplifies URL handling
• Fix app array merging to use appId instead of array index (was causing bugs when configs had apps in different order)
• Add appConfigOnly option to mergeSiteConfig for runtime config behavior
• Add comprehensive unit tests for mergeSiteConfig covering all code paths

Changes

runtime/initialize.test.js

1. Top-level config merge test: Remove siteId, simplify mock, verify merge behavior (runtime overrides build-time, build-time preserved, runtime additions work)
2. App-level config merge test: New test that simulates full flow with addAppConfigs() to verify app-specific config merging works correctly
3. Cleanup: Remove unused newConfig fixture, use fake.url pattern and realistic config shapes

runtime/config/index.ts

1. Fix app array merging: Apps now merge by appId instead of array index using lodash.keyby
2. Add appConfigOnly option: When true, only merges the config property of existing apps (ignores new apps) - used by runtime config since routes/providers can't be sent via JSON

runtime/config/index.test.ts (new)

Comprehensive unit tests for mergeSiteConfig (14 tests):

• Top-level config merging (new values, overrides, preservation)
• App merging with full merge (default behavior)
• App merging with appConfigOnly: true
• Edge cases (empty arrays, undefined apps, missing config)
• Verifies publish(CONFIG_CHANGED) is called in all paths

runtime/initialize.js

• Updated runtimeConfig() to call mergeSiteConfig(data, { appConfigOnly: true })

Test plan

• All 306 tests pass
• New unit tests cover all mergeSiteConfig code paths
• App merging by appId verified (configs with different app order merge correctly)

🤖 Generated with Claude Code

[brian-smith-tcril]

Log

PR 157 Work Log - Runtime Config Test Updates

Date: 2026-02-03

Objective

Update tests in openedx/frontend-base PR openedx#157 to align with the new way of handling runtime config.

---

Progress

Session Start

• Starting work on PR 157 test updates
• Next step: Fetch PR details from GitHub

PR Analysis

PR openedx#157: "feat!: update runtime config support"

Breaking Changes:

• mfeConfigApiUrl renamed to runtimeConfigJsonUrl
• siteId removed from config URL building
• Added cache buster timestamp for development environments

Files Changed:

• runtime/initialize.js - Main implementation changes
• runtime/initialize.test.js - Tests (partially updated)
• Config files and types updated

Test Issues Found:

1. Tests still include siteId: 'auth' in config (no longer used)
2. Mock in configureCache still parses mfe from URL params (old behavior)
3. New cache buster behavior in dev mode not tested

---

Notes

The tests need to be updated to:

1. Remove siteId from test config
2. Simplify the mock to just return data without parsing URL params
3. Optionally add test for cache buster behavior in dev mode

---

Changes Made

runtime/initialize.test.js

1. Renamed "should initialize the app with runtime configuration" to "should merge runtime configuration with build-time configuration":

   • Removed siteId: 'auth' (no longer used)
   • Simplified mock to just return data directly (no more URL param parsing)
   • Updated to test proper merge behavior:
     • Runtime config overrides build-time config for matching keys
     • Build-time values not in runtime are preserved
     • Runtime values not in build-time are added

2. "should initialize the app with the build config when runtime configuration fails" test:

   • Removed siteId: 'auth'
   • Updated URL to .json extension for consistency

3. Added new test "should merge app-level runtime configuration with build-time configuration":

   • Tests that app-level config in apps array merges correctly
   • Simulates full flow by calling addAppConfigs() after initialize
   • Verifies runtime overrides, build-time preservation, and runtime additions at app config level
   • Added imports for addAppConfigs and getAppConfig

4. Removed unused newConfig test fixture:

   • Was previously used for siteId-based filtering tests
   • No longer needed with new test approach

5. Updated test URLs and config values:

   • Use fake.url pattern for test URLs (fake.config.url, fake.cdn.url, fake.lms.url)
   • Use @fake.email for test emails
   • Use realistic theme config shape instead of made-up fields

runtime/config/index.ts

1. Fixed app array merging - apps now merge by appId instead of array index:

   • Discovered that lodash.merge merges arrays by index, not by key
   • This caused problems when build-time and runtime configs had apps in different positions

2. Added appConfigOnly option to mergeSiteConfig:

   • appConfigOnly: false (default, build-time): full app merge by appId, allows adding new apps
   • appConfigOnly: true (runtime): only merge config property for existing apps, ignores new apps
   • Runtime config can't add new apps because routes/providers/etc can't be sent via JSON

3. Implementation details:

   • Uses lodash.keyby to convert app arrays to objects keyed by appId
   • Full merge: Object.values(merge(existingAppsById, newAppsById))
   • Config-only merge: iterate existing apps, lookup by appId, merge only config
   • Added lodash.keyby and @types/lodash.keyby dependencies

runtime/initialize.js

• Updated runtimeConfig() to call mergeSiteConfig(data, { appConfigOnly: true })

---

Commits

1. test: update runtime config test for top-level config merging
2. test: add app-level runtime config merge test
3. refactor: remove unused newConfig test fixture
4. refactor: use realistic config values and fake.url pattern
5. fix: merge apps by appId instead of array index

---

Session 2: 2026-02-04

Objective

Add unit tests for mergeSiteConfig function to cover all code paths, including verifying that publish(CONFIG_CHANGED) is called.

runtime/config/index.test.ts (new file)

Created comprehensive unit tests for mergeSiteConfig with 14 test cases covering:

Top-level config merging:

• Merging new values into site config
• Overriding existing values
• Preserving existing values not in new config

App merging (full merge, default behavior):

• Adding new apps when none exist
• Merging apps by appId, not array index
• Adding new apps while preserving existing ones
• Deep merging app properties

App merging (appConfigOnly: true):

• Does nothing when no existing apps
• Does not add new apps
• Only merges config for existing apps
• Merges by appId, not array index

Edge cases:

• Empty apps array in new config
• Undefined apps in new config
• App with no config property

Mocking publish - Journey to the Solution

We wanted to verify that publish(CONFIG_CHANGED) is called in all code paths. This proved challenging because index.ts captures the publish function reference at module load time.

Attempt 1: Standard jest.mock() with static imports

import { CONFIG_CHANGED } from '../constants';
jest.mock('../subscriptions');
import { publish } from '../subscriptions';
import { getSiteConfig, mergeSiteConfig, setSiteConfig } from './index';

Result: Failed. Even though jest.mock() is hoisted, ./index.ts had already captured the real publish function before the mock took effect. Tests showed "Number of calls: 0".

Attempt 2: jest.resetModules() + jest.doMock() + require()

let publish;
let getSiteConfig, mergeSiteConfig, setSiteConfig;

beforeAll(() => {
  jest.resetModules();
  jest.doMock('../subscriptions', () => ({
    publish: jest.fn(),
    subscribe: jest.fn(),
  }));
  const subscriptions = require('../subscriptions');
  publish = subscriptions.publish;
  const config = require('./index');
  // ... assign functions
});

Result: Works, but uses CommonJS require() which we wanted to avoid.

Attempt 3: jest.resetModules() + jest.doMock() + dynamic import()

beforeAll(async () => {
  jest.resetModules();
  jest.doMock('../subscriptions', () => ({
    publish: jest.fn(),
    subscribe: jest.fn(),
  }));
  const subscriptions = await import('../subscriptions');
  // ...
});

Result: Works, but still requires jest.doMock() in beforeAll.

Attempt 4: jest.mock() at top + dynamic import() in beforeAll

jest.mock('../subscriptions');

beforeAll(async () => {
  const subscriptions = await import('../subscriptions');
  // ...
});

Result: Failed without jest.resetModules(). The module was already cached.

Attempt 5: Same as openedx#4 but with jest.resetModules()

jest.mock('../subscriptions');

beforeAll(async () => {
  jest.resetModules();
  const subscriptions = await import('../subscriptions');
  // ...
});

Result: Works! But still requires let declarations for all imports.

Attempt 6 (Final): jest.spyOn() with namespace import

Looked at existing pattern in useThemeConfig.test.ts:

import * as subscriptions from '../subscriptions';

describe('mergeSiteConfig', () => {
  let publishSpy: jest.SpyInstance;

  beforeEach(() => {
    publishSpy = jest.spyOn(subscriptions, 'publish');
  });

  afterEach(() => {
    publishSpy.mockRestore();
  });

  it('should merge new values', () => {
    mergeSiteConfig({ siteName: 'Test' });
    expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED);
  });
});

Result: Works perfectly! Clean, follows existing codebase patterns, no let declarations needed for the config functions.

Key Learnings

1. jest.mock() hoisting works for mocking modules that the test file imports, but if the module under test imports from the mocked module, it captures the reference at its own load time.

2. jest.spyOn() with namespace imports (import * as module) works because it modifies the actual module object, so all consumers of that module see the spy.

3. The useThemeConfig.test.ts pattern is the established way to do this in the frontend-base codebase.

TypeScript Strict Mode Fix

CI failed due to TypeScript errors - the test file had many "Object is possibly 'undefined'" errors because TypeScript's strict mode couldn't verify that array accesses and .find() results would be defined.

Fix: Added non-null assertions (!) to all places where we access arrays or find results that we know exist in the test context:

• getSiteConfig().apps!
• apps.find(...)!
• app.config!.VALUE

This is acceptable in test code where we're asserting expected behavior.

Wrapped up with a little back and forth about formatting/cleanup.

---

Branch

• Created branch runtime-config-test-updates off of runtime-config
• Pushed to brian-smith-tcril/frontend-base
• PR created: fix: merge apps by appId, update runtime config tests, add mergeSiteConfig unit tests #1