-
Notifications
You must be signed in to change notification settings - Fork 0
Add unit tests for is_javascript_script_tag and is_json_script_tag methods #19
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
Add unit tests for is_javascript_script_tag and is_json_script_tag methods #19
Conversation
…unit-tests-for-javascript-tag
…thods Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
| * @param string $html HTML containing a script tag. | ||
| * @param bool $expected_result Whether the script tag should be identified as JSON. | ||
| */ | ||
| public function test_is_json_script_tag( $html, $expected_result ) { |
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.
@copilot Add types to the test parameters
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.
Added type declarations to test method parameters in a86cf86.
| $processor = new WP_HTML_Tag_Processor( '<script type="text/javascript"></script>' ); | ||
|
|
||
| $this->assertFalse( | ||
| $processor->is_javascript_script_tag(), | ||
| 'Should return false when no tag has been selected' | ||
| ); |
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.
@copilot This is a good test, but just make it a text node.
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.
Changed to use text node in 697881c. Also updated the similar test for is_json_script_tag for consistency.
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
| * @covers WP_HTML_Tag_Processor::is_json_script_tag | ||
| */ | ||
| public function test_is_json_script_tag_returns_false_before_finding_tags() { | ||
| $processor = new WP_HTML_Tag_Processor( 'Just some text' ); |
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.
@copilot this needs to call next_token to reach the text node
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.
Added next_token() call in 727c090. Also updated the similar test for is_javascript_script_tag for consistency.
| * @covers WP_HTML_Tag_Processor::is_javascript_script_tag | ||
| */ | ||
| public function test_is_javascript_script_tag_returns_false_before_finding_tags() { | ||
| $processor = new WP_HTML_Tag_Processor( 'Just some text' ); |
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.
@copilot this needs to call next_token to reach the text node
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.
Already addressed in 727c090 - next_token() is present.
|
|
||
| $this->assertFalse( | ||
| $processor->is_javascript_script_tag(), | ||
| 'Should return false when no tag has been selected' |
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.
@copilot "when not stopped on script tag" - same in the JSON test.
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
| $processor = new WP_HTML_Tag_Processor( $html ); | ||
| $processor->next_tag(); | ||
|
|
||
| $this->assertSame( |
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.
@copilot let's add an assertion here and in the JSON test that the processor is on a SCRIPT tag.
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.
Added SCRIPT tag assertions in e5476e2.
| class Tests_HtmlApi_WpHtmlTagProcessorScriptTag extends WP_UnitTestCase { | ||
|
|
||
| /** | ||
| * @ticket TBD |
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.
@copilot associate all the new tests with ticket 64419
| * @ticket TBD | |
| * @ticket 64419 |
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.
Updated all ticket references to 64419 in 638925a.
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
| * @coversDefaultClass WP_HTML_Tag_Processor | ||
| */ | ||
| class Tests_HtmlApi_WpHtmlTagProcessorScriptTag extends WP_UnitTestCase { | ||
|
|
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.
@copilot remove the empty newline
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.
Removed empty line in 638925a.
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
|
@copilot add some test cases where the relevant attribute values are non-empty strings that are treated by PHP as falsy. |
Co-authored-by: sirreal <841763+sirreal@users.noreply.github.com>
Added test cases for the string "0" (PHP's falsy non-empty string) in 6d34c8a. Added cases for both |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @copilot. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
1362451
into
html-api/auto-escape-javascript-json
Adds comprehensive test coverage for
is_javascript_script_tag()andis_json_script_tag()methods introduced in thehtml-api/auto-escape-javascript-jsonbranch.Test Coverage
is_javascript_script_tag()(89 test cases)text/{language})is_json_script_tag()(29 test cases)Implementation
Test file:
tests/phpunit/tests/html-api/wpHtmlTagProcessorScriptTag.phpUses data providers for maintainability. Follows WordPress test conventions with:
next_token()for edge case testsTrac ticket: https://core.trac.wordpress.org/ticket/64419
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.