-
Notifications
You must be signed in to change notification settings - Fork 3.3k
HTML API: Ensure bookmark exaustion does not error #10616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
sirreal
wants to merge
25
commits into
WordPress:trunk
from
sirreal:html-api/ensure-deep-nesting-no-exception
Closed
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
2c9a17d
Fix group -> ticket test annotation
sirreal da53752
Add failing test for deep nesting
sirreal 0337b13
Handle Exception thrown by bookmark_token()
sirreal 4101235
Add failing test for virtual tokens
sirreal e7d649c
Handle insert_virtual_node exceptions
sirreal 1cfeb85
Fix php 7.x compat
sirreal 17bbca8
Update test ticket annotation
sirreal ba480a5
Increase virtual token HTML size
sirreal ff7657f
Use single-line comment
sirreal b02d679
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal 9e042f3
Return false or throw unsupported exception where available
sirreal c7710aa
Remove general exception catching
sirreal 1681fed
Avoid throwing unsupported exception when tokens exceeded
sirreal 3f6bffd
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal aefc5bf
Remove unsupported union return type
sirreal 6d1f860
Use arrow functions ✨
sirreal 207f04f
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal cbab9c1
Add assertion message and use class constant
sirreal c578125
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal 5e45206
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal c4476be
Restore exception-catching approach
sirreal b847559
Improve tests
sirreal aa06e5e
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal 547d37b
Lints
sirreal b6df5f7
Merge branch 'trunk' into html-api/ensure-deep-nesting-no-exception
sirreal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test confuses me.
Not sure if you noticed, but we should have
$this->assertEqualsWithDelta( MAX_BOOKMARKS, $reached_tokens, 1 );available to say "within 1 of the max"but why the variability? and what does “depending on how the tokens align” mean? it feels flakey or not-fully-understood to me, and I get nervous adding an assertion of that in case we’re testing a happenstance detail and not a contract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is very tricky, and imperfect as it is. I want to confirm that these don't throw errors during processing, which is the big thing I want to address in this PR.
It would be fine to change this to assert that no assertions are thrown.
This test in particular was difficult because the virtual tokens had another failure path. Consider:
<table><td><table><td>produces:To make this test more robust, it would probably need to process 3 times:
<table><td>…<div><table><td>…<div><div><table><td>…Since there are pairs of real tokens
TD (real) + TABLE (real), running with 0, 1, 2 offsets should ensure that the virtual token lands on the threshold at least once.