|
| 1 | +# Audit Request: Angular Builder Integration Tests |
| 2 | + |
| 3 | +## What Was Added |
| 4 | + |
| 5 | +**New Test File:** `src/parameter-tests/builder-integration.spec.ts` |
| 6 | + |
| 7 | +**Purpose:** True integration tests for the Angular Builder execution path (`ng deploy`) |
| 8 | + |
| 9 | +## Audit Questions |
| 10 | + |
| 11 | +### 1. Are These TRUE Integration Tests? |
| 12 | + |
| 13 | +**Claim:** These tests verify the COMPLETE flow from Angular Builder → Engine → Final Options |
| 14 | + |
| 15 | +**Verify:** |
| 16 | +- [ ] Does the test import the REAL `engine` module (not a mock)? |
| 17 | +- [ ] Does the test import the REAL `deploy` function from `deploy/actions.ts`? |
| 18 | +- [ ] Are ONLY external dependencies mocked (gh-pages, fs-extra)? |
| 19 | +- [ ] Does the test execute the actual transformation logic in `engine.prepareOptions()`? |
| 20 | + |
| 21 | +**How to check:** |
| 22 | +```typescript |
| 23 | +// Line 80-81: Are these the real imports? |
| 24 | +import deploy from '../deploy/actions'; |
| 25 | +import * as engine from '../engine/engine'; |
| 26 | + |
| 27 | +// Line 95: Is this passing the REAL engine (not a mock)? |
| 28 | +await deploy(engine, context, BUILD_TARGET, options); |
| 29 | +``` |
| 30 | + |
| 31 | +### 2. Does Mocking Strategy Make Sense? |
| 32 | + |
| 33 | +**Claim:** We mock external dependencies but NOT our internal code |
| 34 | + |
| 35 | +**Verify:** |
| 36 | +- [ ] Is `gh-pages` mocked? (Lines 24-39) |
| 37 | +- [ ] Is `fs-extra` mocked? (Lines 57-77) |
| 38 | +- [ ] Is `engine` NOT mocked? |
| 39 | +- [ ] Is `deploy/actions` NOT mocked? |
| 40 | + |
| 41 | +**Why this matters:** |
| 42 | +- External deps (gh-pages, fs) = side effects we want to avoid |
| 43 | +- Internal code (engine, deploy) = what we're testing |
| 44 | + |
| 45 | +### 3. Are All Critical Parameters Tested? |
| 46 | + |
| 47 | +**Claim:** All boolean negation transformations are verified in complete flow |
| 48 | + |
| 49 | +**Verify:** |
| 50 | +- [ ] Test: `noDotfiles: true` → `dotfiles: false` (Line 88-101) |
| 51 | +- [ ] Test: `noNotfound: true` → `notfound: false` (Line 103-116) |
| 52 | +- [ ] Test: `noNojekyll: true` → `nojekyll: false` (Line 118-131) |
| 53 | +- [ ] Test: All three together (Line 133-153) |
| 54 | +- [ ] Test: Default values when not specified (Line 155-167) |
| 55 | + |
| 56 | +**Critical check:** Do tests verify BOTH: |
| 57 | +1. The transformed property (`dotfiles: false`) |
| 58 | +2. AND that the original property is still present (`noDotfiles: true`) |
| 59 | + |
| 60 | +### 4. Are Tests Actually Running Real Code? |
| 61 | + |
| 62 | +**How to verify:** |
| 63 | + |
| 64 | +Run the tests and check they actually call the engine: |
| 65 | + |
| 66 | +```bash |
| 67 | +npm test -- builder-integration.spec.ts |
| 68 | +``` |
| 69 | + |
| 70 | +**Add a console.log to `engine/engine.ts` line 83:** |
| 71 | +```typescript |
| 72 | +if (origOptions.noDotfiles) { |
| 73 | + console.log('AUDIT: Engine is transforming noDotfiles'); |
| 74 | + options.dotfiles = !origOptions.noDotfiles; |
| 75 | +} |
| 76 | +``` |
| 77 | + |
| 78 | +**Run test again:** |
| 79 | +```bash |
| 80 | +npm test -- builder-integration.spec.ts 2>&1 | grep "AUDIT" |
| 81 | +``` |
| 82 | + |
| 83 | +**Expected:** You should see "AUDIT: Engine is transforming noDotfiles" |
| 84 | + |
| 85 | +**If you DON'T see it:** The test is NOT running real code! |
| 86 | + |
| 87 | +### 5. Do Tests Capture Final Options Correctly? |
| 88 | + |
| 89 | +**Claim:** We capture the exact options that would be passed to `gh-pages.publish()` |
| 90 | + |
| 91 | +**Verify:** |
| 92 | +- [ ] Check mock at lines 24-39: Does it capture options correctly? |
| 93 | +- [ ] Check test assertions: Do they verify `capturedPublishOptions`? |
| 94 | +- [ ] Run a test and add debug output to confirm options are captured |
| 95 | + |
| 96 | +**How to verify:** |
| 97 | +```typescript |
| 98 | +// Add to beforeEach (line 86): |
| 99 | +console.log('CAPTURED OPTIONS:', JSON.stringify(capturedPublishOptions, null, 2)); |
| 100 | +``` |
| 101 | + |
| 102 | +Run test and verify output shows the transformed options. |
| 103 | + |
| 104 | +### 6. Are Tests Independent? |
| 105 | + |
| 106 | +**Verify:** |
| 107 | +- [ ] Does `beforeEach` reset `capturedPublishOptions = null`? (Line 84) |
| 108 | +- [ ] Do tests not interfere with each other? |
| 109 | +- [ ] Can tests run in any order? |
| 110 | + |
| 111 | +**How to verify:** |
| 112 | +```bash |
| 113 | +# Run tests multiple times |
| 114 | +npm test -- builder-integration.spec.ts |
| 115 | +npm test -- builder-integration.spec.ts |
| 116 | +npm test -- builder-integration.spec.ts |
| 117 | +``` |
| 118 | + |
| 119 | +All runs should pass identically. |
| 120 | + |
| 121 | +### 7. Do Tests Match Reality? |
| 122 | + |
| 123 | +**Critical question:** Do the integration tests produce the same results as the CLI E2E tests? |
| 124 | + |
| 125 | +**Compare:** |
| 126 | + |
| 127 | +**CLI E2E test** (`cli-e2e.spec.ts` line 111-117): |
| 128 | +```typescript |
| 129 | +it('should handle --no-dotfiles flag', () => { |
| 130 | + const output = runCli('--no-dotfiles'); |
| 131 | + const json = parseJsonFromCliOutput(output); |
| 132 | + expect(json.dotfiles).toBe("files starting with dot ('.') will be ignored"); |
| 133 | +}); |
| 134 | +``` |
| 135 | + |
| 136 | +**Builder Integration test** (`builder-integration.spec.ts` line 88-101): |
| 137 | +```typescript |
| 138 | +it('should transform noDotfiles: true to dotfiles: false in complete flow', async () => { |
| 139 | + const options: Schema = { |
| 140 | + repo: 'https://github.com/test/repo.git', |
| 141 | + noDotfiles: true, |
| 142 | + noBuild: true |
| 143 | + }; |
| 144 | + |
| 145 | + await deploy(engine, context, BUILD_TARGET, options); |
| 146 | + |
| 147 | + expect(capturedPublishOptions).not.toBeNull(); |
| 148 | + expect(capturedPublishOptions.dotfiles).toBe(false); |
| 149 | +}); |
| 150 | +``` |
| 151 | + |
| 152 | +**Verify:** |
| 153 | +- [ ] Both tests verify `dotfiles` property |
| 154 | +- [ ] CLI test checks dry-run output message |
| 155 | +- [ ] Integration test checks actual option value |
| 156 | +- [ ] Both should produce equivalent results (CLI shows message, Integration shows value) |
| 157 | + |
| 158 | +### 8. Code Coverage Question |
| 159 | + |
| 160 | +**Does this test file increase coverage?** |
| 161 | + |
| 162 | +Compare test coverage before and after: |
| 163 | + |
| 164 | +```bash |
| 165 | +# Check what these tests actually execute |
| 166 | +npm test -- builder-integration.spec.ts --coverage --collectCoverageFrom='src/engine/**/*.ts' --collectCoverageFrom='src/deploy/**/*.ts' |
| 167 | +``` |
| 168 | + |
| 169 | +**Verify:** |
| 170 | +- [ ] Does coverage report show `engine.ts` is executed? |
| 171 | +- [ ] Does coverage report show `prepareOptions()` is executed? |
| 172 | +- [ ] Are the transformation lines (79-91) covered? |
| 173 | + |
| 174 | +## Expected Audit Results |
| 175 | + |
| 176 | +### ✅ PASS Criteria |
| 177 | + |
| 178 | +1. Tests use REAL engine and deploy/actions (not mocked) |
| 179 | +2. Tests execute REAL transformation logic |
| 180 | +3. Only external dependencies are mocked |
| 181 | +4. All critical boolean negation parameters tested |
| 182 | +5. Tests verify complete flow from input → transformation → final output |
| 183 | +6. Tests are independent and repeatable |
| 184 | +7. Adding console.log to engine shows it's being called |
| 185 | +8. Coverage report confirms engine code is executed |
| 186 | + |
| 187 | +### ❌ FAIL Criteria |
| 188 | + |
| 189 | +1. Engine or deploy/actions are mocked |
| 190 | +2. Transformation logic is bypassed |
| 191 | +3. Tests don't actually call `prepareOptions()` |
| 192 | +4. Missing critical parameter tests |
| 193 | +5. Tests share state or aren't independent |
| 194 | +6. Adding console.log to engine shows nothing (not being called) |
| 195 | + |
| 196 | +## How to Run This Audit |
| 197 | + |
| 198 | +```bash |
| 199 | +cd src |
| 200 | + |
| 201 | +# 1. Run tests to verify they pass |
| 202 | +npm test -- builder-integration.spec.ts |
| 203 | + |
| 204 | +# 2. Add debug output to engine/engine.ts (line 83) |
| 205 | +# Add: console.log('AUDIT: Transforming noDotfiles =', origOptions.noDotfiles); |
| 206 | + |
| 207 | +# 3. Run tests again to see debug output |
| 208 | +npm test -- builder-integration.spec.ts 2>&1 | grep "AUDIT" |
| 209 | + |
| 210 | +# 4. Check coverage |
| 211 | +npm test -- builder-integration.spec.ts --coverage |
| 212 | + |
| 213 | +# 5. Remove debug output from engine.ts |
| 214 | + |
| 215 | +# 6. Verify all tests still pass |
| 216 | +npm test |
| 217 | +``` |
| 218 | + |
| 219 | +## Questions for Auditor |
| 220 | + |
| 221 | +1. **Are these true integration tests or disguised unit tests?** |
| 222 | + |
| 223 | +2. **Is our mocking strategy correct?** (Mock externals, not internals) |
| 224 | + |
| 225 | +3. **Do these tests prove that Angular Builder and CLI paths work identically?** |
| 226 | + |
| 227 | +4. **Are we missing any critical test cases?** |
| 228 | + |
| 229 | +5. **Is there a better way to verify integration without mocking?** |
| 230 | + |
| 231 | +## Auditor Sign-Off |
| 232 | + |
| 233 | +**Auditor:** _____________________ |
| 234 | + |
| 235 | +**Date:** _____________________ |
| 236 | + |
| 237 | +**Result:** [ ] PASS / [ ] FAIL / [ ] NEEDS IMPROVEMENT |
| 238 | + |
| 239 | +**Comments:** |
| 240 | + |
| 241 | +--- |
| 242 | +--- |
| 243 | +--- |
| 244 | + |
| 245 | +**Findings:** |
| 246 | + |
| 247 | +**Recommendations:** |
0 commit comments