Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions DEFECTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions addon/components/country-select.js
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
29 changes: 27 additions & 2 deletions tests/integration/components/coordinates-input-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -156,8 +160,10 @@ module('Integration | Component | coordinates-input', function (hooks) {
await render(hbs`<CoordinatesInput @onInit={{this.onInit}} @onGeocode={{this.onGeocode}} />`);

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');
});

Expand Down Expand Up @@ -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`<CoordinatesInput @onInit={{this.onInit}} @onChange={{this.onChange}} />`);

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');
});
});
18 changes: 18 additions & 0 deletions tests/integration/components/country-select-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`<CountrySelect />`);
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`<CountrySelect />`);
await selectChoose('.fleetbase-power-select', 'Singapore');

assert.dom('.ember-power-select-trigger').includesText('Singapore', 'the selection is applied without a listener');
});
});