fix: explain a rejected dust threshold instead of silently snapping back (closes #233)
All checks were successful
check / check (push) Successful in 33s
All checks were successful
check / check (push) Successful in 33s
The dust-threshold field was the only validated input in Settings that rejected without saying anything: the value silently changed back to the stored one with no explanation. It now flashes "Please enter a whole number of gwei, zero or greater." alongside the existing resync, matching the idiom the RPC URL field already uses. The parse moves to its own module and accepts plain decimal digits only, zero or greater. Hex and exponent notation are refused rather than accepted: Number() reads "0x10" as 16 and "1e3" as 1000, neither of which the previous parseInt produced, and storing a number the user did not type is the same silent substitution this change exists to remove. The message must fit one line of the reserved flash area -- a wrapped message pushes the settings view down, which the No Layout Shift policy forbids. That is pinned by an end-to-end test measuring the rendered line height and the position of the elements below it, in a single round trip because the flash clears after two seconds.
This commit was merged in pull request #243.
This commit is contained in:
243
tests/dustThreshold.test.js
Normal file
243
tests/dustThreshold.test.js
Normal file
@@ -0,0 +1,243 @@
|
||||
// Tests for the dust threshold field in Settings (issue #233).
|
||||
//
|
||||
// Two halves: what the parse accepts, and what the settings view does with a
|
||||
// rejection. The view half runs against the real change handler with the DOM
|
||||
// helpers stubbed out, because the bug was not in the parse — it was that a
|
||||
// rejection said nothing.
|
||||
|
||||
const {
|
||||
DUST_THRESHOLD_MESSAGE,
|
||||
parseDustThresholdGwei,
|
||||
} = require("../src/popup/dustThreshold");
|
||||
|
||||
describe("parsing the dust threshold", () => {
|
||||
test("accepts a whole number of gwei", () => {
|
||||
expect(parseDustThresholdGwei("100000")).toBe(100000);
|
||||
expect(parseDustThresholdGwei("1")).toBe(1);
|
||||
});
|
||||
|
||||
// Zero is a real setting, not an empty field: it hides nothing.
|
||||
test("accepts zero", () => {
|
||||
expect(parseDustThresholdGwei("0")).toBe(0);
|
||||
});
|
||||
|
||||
test("accepts surrounding whitespace", () => {
|
||||
expect(parseDustThresholdGwei(" 250 ")).toBe(250);
|
||||
});
|
||||
|
||||
test("rejects an empty field", () => {
|
||||
expect(parseDustThresholdGwei("")).toBe(null);
|
||||
expect(parseDustThresholdGwei(" ")).toBe(null);
|
||||
});
|
||||
|
||||
test("rejects a negative threshold", () => {
|
||||
expect(parseDustThresholdGwei("-1")).toBe(null);
|
||||
});
|
||||
|
||||
// parseInt used to read this as 1, which is not what was typed.
|
||||
test("rejects a fractional value", () => {
|
||||
expect(parseDustThresholdGwei("1.5")).toBe(null);
|
||||
expect(parseDustThresholdGwei("1.0")).toBe(null);
|
||||
});
|
||||
|
||||
// parseInt used to read this as 100. The unit is printed beside the
|
||||
// field already.
|
||||
test("rejects a value carrying its unit", () => {
|
||||
expect(parseDustThresholdGwei("100 gwei")).toBe(null);
|
||||
});
|
||||
|
||||
// Number() reads this as 16. Storing 16 for a field that was told to
|
||||
// want a whole number of gwei would be the same silent substitution the
|
||||
// message exists to end.
|
||||
test("rejects hex notation", () => {
|
||||
expect(parseDustThresholdGwei("0x10")).toBe(null);
|
||||
});
|
||||
|
||||
// Number() reads this as 1000.
|
||||
test("rejects exponent notation", () => {
|
||||
expect(parseDustThresholdGwei("1e3")).toBe(null);
|
||||
});
|
||||
|
||||
test("rejects other non-numeric input", () => {
|
||||
expect(parseDustThresholdGwei("lots")).toBe(null);
|
||||
expect(parseDustThresholdGwei("+5")).toBe(null);
|
||||
expect(parseDustThresholdGwei("Infinity")).toBe(null);
|
||||
expect(parseDustThresholdGwei(undefined)).toBe(null);
|
||||
expect(parseDustThresholdGwei(5)).toBe(null);
|
||||
});
|
||||
|
||||
// Beyond 2^53 the digits would round on the way in, so the stored
|
||||
// threshold would not be the one typed.
|
||||
test("rejects a value too large to hold exactly", () => {
|
||||
expect(parseDustThresholdGwei("9007199254740993")).toBe(null);
|
||||
});
|
||||
});
|
||||
|
||||
describe("the rejection message", () => {
|
||||
// README, Language & Labeling: error messages are full sentences.
|
||||
test("is a full sentence naming the constraint", () => {
|
||||
expect(DUST_THRESHOLD_MESSAGE).toMatch(/^[A-Z].*\.$/);
|
||||
expect(DUST_THRESHOLD_MESSAGE).toContain("whole number of gwei");
|
||||
expect(DUST_THRESHOLD_MESSAGE).toContain("zero or greater");
|
||||
});
|
||||
});
|
||||
|
||||
describe("the flash line the message is shown in", () => {
|
||||
const fs = require("fs");
|
||||
const path = require("path");
|
||||
|
||||
const POPUP_HTML = fs.readFileSync(
|
||||
path.join(__dirname, "..", "src", "popup", "index.html"),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
// This asserts only that the reservation exists in the markup. It does
|
||||
// NOT and CANNOT assert that the message fits inside it: jest runs on
|
||||
// the node environment here, with no layout engine, so every rendered
|
||||
// height is zero. An earlier version of this block claimed to pin the
|
||||
// No Layout Shift policy with this regex, and it passed at any message
|
||||
// length, including one that wrapped to two lines and pushed the
|
||||
// settings view down 12px.
|
||||
//
|
||||
// The assertion that actually measures — empty line vs. the message,
|
||||
// real Chromium, documented 360x600 popup — is
|
||||
// "a rejected dust threshold shifts no layout (#233)" in
|
||||
// tests/e2e/run.js, run by make test-e2e. It is not in make check
|
||||
// because REPO_POLICIES.md caps make test at 20 seconds and a browser
|
||||
// suite does not fit; run it before changing the wording.
|
||||
test("reserves its height in the markup", () => {
|
||||
const flashLine = POPUP_HTML.match(
|
||||
/<div\s+id="flash-msg"\s+class="([^"]*)"/,
|
||||
);
|
||||
expect(flashLine).not.toBeNull();
|
||||
expect(flashLine[1]).toMatch(/min-h-\[/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("the settings view on a change to the field", () => {
|
||||
let elements;
|
||||
let flashes;
|
||||
let saves;
|
||||
let state;
|
||||
|
||||
// A stand-in for one DOM node: enough of an element for init() to set
|
||||
// properties on it and hang listeners off it.
|
||||
function fakeElement() {
|
||||
return {
|
||||
value: "",
|
||||
checked: false,
|
||||
textContent: "",
|
||||
href: "",
|
||||
style: {},
|
||||
dataset: {},
|
||||
classList: { add() {}, remove() {} },
|
||||
listeners: {},
|
||||
addEventListener(event, handler) {
|
||||
this.listeners[event] = handler;
|
||||
},
|
||||
querySelectorAll: () => [],
|
||||
};
|
||||
}
|
||||
|
||||
function loadSettingsView() {
|
||||
elements = {};
|
||||
flashes = [];
|
||||
saves = 0;
|
||||
|
||||
jest.resetModules();
|
||||
|
||||
jest.doMock("../src/popup/views/helpers", () => ({
|
||||
$: (id) => (elements[id] ||= fakeElement()),
|
||||
showView: () => {},
|
||||
updateDebugBanner: () => {},
|
||||
showFlash: (msg) => flashes.push(msg),
|
||||
escapeHtml: (s) => s,
|
||||
flashCopyFeedback: () => {},
|
||||
goBack: () => {},
|
||||
pushCurrentView: () => {},
|
||||
onViewLeave: () => {},
|
||||
VIEWS: [],
|
||||
}));
|
||||
|
||||
state = require("../src/shared/state").state;
|
||||
state.dustThresholdGwei = 100000;
|
||||
|
||||
const settings = require("../src/popup/views/settings");
|
||||
settings.init({});
|
||||
return elements["settings-dust-threshold"];
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
globalThis.chrome = {
|
||||
runtime: { sendMessage: () => {} },
|
||||
storage: {
|
||||
local: {
|
||||
get: async () => ({}),
|
||||
set: async () => {
|
||||
saves++;
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
jest.dontMock("../src/popup/views/helpers");
|
||||
delete globalThis.chrome;
|
||||
});
|
||||
|
||||
async function change(field, typed) {
|
||||
field.value = typed;
|
||||
await field.listeners.change();
|
||||
}
|
||||
|
||||
test("a valid value is stored and says nothing", async () => {
|
||||
const field = loadSettingsView();
|
||||
|
||||
await change(field, "250");
|
||||
|
||||
expect(state.dustThresholdGwei).toBe(250);
|
||||
expect(field.value).toBe(250);
|
||||
expect(flashes).toEqual([]);
|
||||
expect(saves).toBe(1);
|
||||
});
|
||||
|
||||
test("a rejected value shows the message and is not stored", async () => {
|
||||
const field = loadSettingsView();
|
||||
|
||||
await change(field, "1.5");
|
||||
|
||||
expect(state.dustThresholdGwei).toBe(100000);
|
||||
expect(flashes).toEqual([DUST_THRESHOLD_MESSAGE]);
|
||||
expect(saves).toBe(0);
|
||||
});
|
||||
|
||||
// The snap-back is the behaviour the message explains, so it stays.
|
||||
test("a rejected value still resyncs the field to what is stored", async () => {
|
||||
const field = loadSettingsView();
|
||||
|
||||
await change(field, "100 gwei");
|
||||
|
||||
expect(field.value).toBe(100000);
|
||||
});
|
||||
|
||||
test("every rejected notation gets the same one message", async () => {
|
||||
for (const typed of ["", "-1", "1.5", "100 gwei", "0x10", "1e3"]) {
|
||||
const field = loadSettingsView();
|
||||
|
||||
await change(field, typed);
|
||||
|
||||
expect(flashes).toEqual([DUST_THRESHOLD_MESSAGE]);
|
||||
expect(state.dustThresholdGwei).toBe(100000);
|
||||
}
|
||||
});
|
||||
|
||||
test("zero is accepted, not treated as an empty field", async () => {
|
||||
const field = loadSettingsView();
|
||||
|
||||
await change(field, "0");
|
||||
|
||||
expect(state.dustThresholdGwei).toBe(0);
|
||||
expect(flashes).toEqual([]);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user