search

fgnass / domino

Server-side DOM implementation based on Mozilla's dom.js
BSD 2-Clause "Simplified" License
764 stars 118 forks source link

Fix $= selector #135

Closed dstillman closed 5 years ago

dstillman commented 5 years ago

Zest's $= check used indexOf() without checking for -1 and then did a length check, so if the test string happened to be one character longer than the attribute value, it returned true, even if the string didn't match.

Nothing really to put in a test here, since this was just an edge case.

cscott commented 5 years ago

Even though a test case seems trivial, it would be appreciated. Just add a simple doc and a document.querySelector() call that fails without this patch but succeeds with it...

dstillman commented 5 years ago

It's not that a test is trivial — it's just that the bug here was a random coding error that would be very unlikely to be reproduced in new code, and adding a test for every possible permutation of how something could hypothetically be broken isn't necessarily desirable. Anyhow, I've added a test.

cscott commented 5 years ago

I agree -- it's just that tests are also useful documentation for what was fixed and why. Someone trying to figure out if this bug could have affected them might be helped by seeing an example, for instance. And it also helps convince the maintainer that your patch does what it says on the tin -- my human eyes are most easily tricked by patches which look "obviously correct", so those are the ones I like especially to see test cases for.