Repository navigation
test(compact): cover the Do-Not-Sell row, the one compact rule nothing asserted - #313
Merged
Merged
Conversation
…g asserted test-mobile-compact-css-php.php asserts every rule the compact phone layout emits — the flex basis, the 44px tap target, the wrapping, the order reset, the classic chevron padding — except the Do-Not-Sell control, which had no unit assertion at all. Only the E2E spec covered it, and that does not run in `npm run test:unit`. That control is the fourth item in the wrapper and the one the compared-pair rules do not describe, so each property is load-bearing: without a full basis it shares the row with accept and reject, and at 360px their 40% bases leave it almost nothing to grow into — it collapsed to a few pixels while its label spilled across Accept. It is reached in ordinary operation, not only by hand: Geo_Runtime turns donotSell on for a US visitor while applicableLaw stays 'gdpr', so for that visitor this row IS the opt-out. Six assertions per banner type (+36 overall, 61 -> 97): - the rule exists at all; - flex:1 1 100% and width:100%, so it takes a row instead of a share of one; - white-space:normal, because "Do Not Sell or Share My Personal Information" is statutory wording that cannot be shortened and does not fit one phone line; - display:block, which keeps the template's left-aligned link look rather than the centred flex box the compared buttons use; - the selector matches the data attribute and NOT the class, because the shortcode emits either `.faz-btn.faz-btn-do-not-sell` or a bare <a> with no class, and narrowing it would silently drop the link variant; - the rule comes AFTER the generic `.faz-btn` rule it overrides. Both selectors are 1-1-0, so source order alone decides the winner — a reorder that changes no rule text would restore the collapse invisibly. Each one was control-run: a full basis swapped back to `flex:1 1 0`, the selector narrowed to the class, white-space dropped, and the rule moved above the generic one each turn the matching assertion red (4, 28, 4 and 4 respectively), and the source-order control turns red on that assertion alone. Two further fixes were examined and deliberately left untested, rather than covered with assertions that would pass regardless: - clearstatcache() in Geolocation::database_fingerprint(). Removing it keeps every test green because the premise does not reproduce here: on PHP 8.4 filesize() returns the new length after file_put_contents() and even after an external writer, so the stale-stat condition the line defends against is not observable. Worth knowing that the existing "database replaced -> different fingerprint" assertion does not prove that line either: it writes through a helper that clears the stat cache itself. - the `degraded` copy that distinguishes probe_failed from key_without_database. The reason code it branches on already has three unit assertions; reaching the state from the browser needs a fake MaxMind licence key and a deliberately broken database planted on the test site, which is a poor trade for one `if` on a string.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues. Two small rough edges, both in the new test.
Reviewed changes
I reviewed the new unit coverage for the compact-layout Do-Not-Sell rule against the production rule at frontend/class-frontend.php ([data-faz-tag="donotsell-button"]{flex:1 1 100%;width:100%;display:block;white-space:normal;}). The suite passes locally (97/97).
- Do-Not-Sell rule pinned per banner type: the new checks cover the rule's existence, its four declarations, the attribute-based selector, and that it comes after the generic
.faz-btnrule in source order.
ℹ️ Nitpicks
tests/unit/test-mobile-compact-css-php.php:191— the comment calls both selectors1-1-0, but#faz-consent .faz-notice-btn-wrapper .faz-btnis one id plus two classes, so it is1-2-0. The attribute selector also counts as a class, so the DNS rule is1-2-0as well. The conclusion still holds because the two are tied and source order decides, and line 145 of the same file already uses1-2-0for the equivalent selector.
claude-opus-5-5 ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was missing
test-mobile-compact-css-php.phpasserts every rule the compact phone layout emits — flex basis, the 44px tap target, wrapping, theorderreset, the classic chevron padding — except the Do-Not-Sell control, which had no unit assertion at all. Onlymobile-compact-layout.spec.tscovered it, and that does not run innpm run test:unit.That control is the fourth item in the wrapper and the one the compared-pair rules do not describe, so each property is load-bearing. Without a full basis it shares the row with accept and reject, and at 360px their 40% bases leave it almost nothing to grow into: it collapses to a few pixels while its label spills across Accept.
It is reached in ordinary operation, not only by hand —
Geo_RuntimeturnsdonotSellon for a US visitor whileapplicableLawstaysgdpr, andclass-template.phpkeeps the button precisely because its status is true. For that visitor, this row is the opt-out.Added
Six assertions per banner type (61 → 97 overall):
flex:1 1 100%+width:100%white-space:normaldisplay:block<a>variant is silently dropped.faz-btnruleControl runs
Each assertion was verified to fail when its property is removed:
flex:1 1 100%→flex:1 1 0.faz-btn-do-not-sellwhite-space:normaldroppedThe last one matters most: it catches a reorder that changes no rule text.
197/197unit suites pass; production files are byte-identical tomainafter every control.Two fixes deliberately left untested
Rather than add assertions that would pass regardless:
clearstatcache()inGeolocation::database_fingerprint()— removing it keeps every test green, because the premise does not reproduce here: on PHP 8.4filesize()returns the new length afterfile_put_contents()and even after an external writer, so the stale-stat condition is not observable. Worth recording that the existing "database replaced → different fingerprint" assertion does not prove that line either — it writes through a helper that clears the stat cache itself, so it would stay green with the production line gone.degradedcopy that distinguishesprobe_failedfromkey_without_database— the reason code it branches on already has three unit assertions. Reaching the state from a browser needs a fake MaxMind licence key and a deliberately broken database planted on the test site, which is a poor trade for oneifon a string.