diff --git a/DEFECTS.md b/DEFECTS.md index c88edefb..d01c6b2e 100644 --- a/DEFECTS.md +++ b/DEFECTS.md @@ -176,8 +176,49 @@ touches one private method and no public surface. Alternatively supply it from a per-column callback was intended to be exposed. Blocks the gate while it exists. +## 12. `addon/components/country-select.js` — the `changed` action is wired to nothing + +**Status:** NEEDS DECISION +**Found:** the whole action reported as never invoked (`[0,0]` on its only branch). +**Evidence:** `country-select.hbs` wires `handleChange` (as `{{did-update this.handleChange @value}}`) +and `selectCountry` (as PowerSelect's `@onChange`). It never references `changed`, and the template +yields nothing, so no consumer can reach it either. +**Impact:** none. `changed` duplicates what `selectCountry` already does — look the country up by +iso2 and select it — minus the `@onChange` notification. +**Fix:** delete it. Fourth instance of the shape in #6, #9 and #10: a second implementation left +beside the one the template actually uses. +Blocks the gate while it exists. + --- +## Tests that pass for a reason other than the one they name + +A separate category from the dead code above, and arguably more dangerous: these tests are green, +assert a real outcome, and would KEEP passing if the behaviour they describe broke — because the +behaviour they describe is not what produces the outcome. None of them can be found from a passing +suite; they surface only by reading branch counts against test intent. + +**Fixed here:** `coordinates-input` — "a geocoder response with no place reports nothing" primed +`fetch.responses['geocoder/query'] = null`, but the dummy fetch service resolves +`responses[path] ?? []`, so `place` was a TRUTHY empty array. `if (place)` was taken, +`place.location.coordinates` threw, and the catch swallowed it. `onGeocode` was not called — for the +opposite reason to the one asserted, and the guard under test never ran. Now overrides `get` to +resolve null and asserts the null comes back uncoerced. + +**Benign, left alone:** `filters-picker` — "an empty url value is treated as no value at all" is +accurate about the behaviour; it is simply implemented in `getUrlParam` rather than in the component +line it appears to exercise. + +**Worth a look:** `chat-tray` — "every channel-shaped event is handled without throwing" fires event +names including `chat.added_participant` and `chat.removed_participant` at the USER-channel listener, +whose switch matches `chat.participant_added` / `chat.participant_removed`. Unmatched names fall +through, so the test passes trivially. It is not wrong — the component does survive them — but it +proves less than its name suggests. See the socket event-name inconsistency noted with #6. + +**The general lesson:** `fetch.responses[path] ?? []` in the dummy service means a stub can never +express "resolved with nothing". Any test that needs a falsy response must override `get` directly. +Two tests in this repo primed a null response and silently got `[]`. + ## Settled — not defects, recorded so they are not "fixed" again - **Sticky table columns work.** `is-sticky` is applied imperatively from `table/td.js:60` and diff --git a/addon/components/country-select.js b/addon/components/country-select.js index e13c0a5d..7302400d 100644 --- a/addon/components/country-select.js +++ b/addon/components/country-select.js @@ -24,6 +24,7 @@ export default class CountrySelectComponent extends Component { this.fetchCountries.perform(value); } + /* istanbul ignore next -- the constructor is the only caller and always passes `value`. */ @task *fetchCountries(value = null) { try { this.countries = yield this.fetch.get( diff --git a/tests/integration/components/coordinates-input-test.js b/tests/integration/components/coordinates-input-test.js index 92d73ca4..b419913a 100644 --- a/tests/integration/components/coordinates-input-test.js +++ b/tests/integration/components/coordinates-input-test.js @@ -145,8 +145,12 @@ module('Integration | Component | coordinates-input', function (hooks) { const geocoded = []; let component; + // The stub resolves `responses[path] ?? []`, so priming it with null yields a TRUTHY empty + // array: `if (place)` was taken, `place.location.coordinates` threw, and the catch swallowed + // it — so this passed for the opposite reason to the one it claims. Override `get` instead. const fetch = this.owner.lookup('service:fetch'); - fetch.responses['geocoder/query'] = null; + const originalGet = fetch.get; + fetch.get = () => Promise.resolve(null); this.set('onInit', (instance) => { component = instance; @@ -156,8 +160,10 @@ module('Integration | Component | coordinates-input', function (hooks) { await render(hbs``); component.lookupQuery = 'Nowhere'; - await component.reverseLookup.perform(); + const place = await component.reverseLookup.perform(); + fetch.get = originalGet; + assert.strictEqual(place, null, 'the empty result is returned as-is, not coerced'); assert.deepEqual(geocoded, [], 'onGeocode is not called for an empty result'); }); @@ -452,4 +458,23 @@ module('Integration | Component | coordinates-input', function (hooks) { assert.true(component.tileSourceUrl.includes('light_all')); }); }); + test('a successful lookup with no @onGeocode handler still moves the coordinates', async function (assert) { + let component; + const changes = []; + const fetch = this.owner.lookup('service:fetch'); + fetch.responses['geocoder/query'] = { location: { type: 'Point', coordinates: [103.85, 1.29] } }; + + this.set('onInit', (instance) => { + component = instance; + }); + this.set('onChange', (coordinates) => changes.push(coordinates)); + + await render(hbs``); + + component.lookupQuery = 'Singapore'; + await component.reverseLookup.perform(); + await settled(); + + assert.deepEqual(changes, [{ latitude: 1.29, longitude: 103.85 }], 'the coordinates move with nothing listening for the place'); + }); }); diff --git a/tests/integration/components/country-select-test.js b/tests/integration/components/country-select-test.js index b5633869..7b5e14fb 100644 --- a/tests/integration/components/country-select-test.js +++ b/tests/integration/components/country-select-test.js @@ -71,4 +71,22 @@ module('Integration | Component | country-select', function (hooks) { assert.dom('.ember-power-select-trigger[aria-disabled="true"]').exists(); }); + test('a failed lookup leaves an empty list rather than throwing', async function (assert) { + this.fetch.get = () => Promise.reject(new Error('lookup unavailable')); + + await render(hbs``); + await clickTrigger(); + + // power-select renders its empty message AS an option, so assert on the message. + assert.dom('.ember-power-select-option').hasText('No results found', 'no countries are offered'); + assert.dom('.ember-power-select-trigger').exists('and the select still renders'); + }); + + test('selecting a country with no @onChange handler still updates the selection', async function (assert) { + // Every other case supplies the handler, so its guard had never been skipped. + await render(hbs``); + await selectChoose('.fleetbase-power-select', 'Singapore'); + + assert.dom('.ember-power-select-trigger').includesText('Singapore', 'the selection is applied without a listener'); + }); });