-
Notifications
You must be signed in to change notification settings - Fork 7
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
Tests: improve existing tests #34
Conversation
... as PHPUnit < 5.7.21 doesn't contain the forward compatibility layer with namespaced classes.
* Add more detailed basic test configuration * Unless `junit` is used somewhere, generating this report for code coverage is unnecessary. * Generating the text report, however, is useful.
Realistically, all tests in the `HighligherTest` file are testing the handling of various token types and only incidentally testing the rest of the logic of the `Highlighter` class. With that in mind, I'm proposing to rename the test file to make it more obvious what is being tested. This will then also allow for adding additional new test classes to test the rest of the functionality of the `Highlighter` class. Includes adding minimal documentation and a minor method order tweak.
For most tests, the text within the heredoc should _not_ be interpreted (interpolated). For those tests, it makes more sense to use nowdocs instead of heredocs. Similar to single quoted versus double quoted strings, text within nowdocs is not interpolated (variables are not expanded), while within heredocs, they are. Nowdocs are available since PHP 5.3, so can be safely used within this test suite. Refs: * https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.heredoc * https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.nowdoc
This sets up a new structure for this test class using test methods with data providers. All other existing tests will be converted to data sets in data providers in follow on commits.
Note that the new test data uses NOWDOC format instead of HEREDOC format.
Note: this removes the looping from the test (which was a bad thing). Advantages of using data providers compared to using a loop within the test function, are: 1. When one of the "test cases" fails, the rest of the tests cases will still be run (not so when using a loop). This prevents one test failure "hiding" behind another failure. 2. When there are multiple assertions is a test, it is often hard to determine which of the assertions (test cases) failed. Using a data provider - especially with named cases like used in this commit - will make the error coming from PHPUnit more descriptive and will make debugging which test case failed easier. 3. The test cases will now show (and be counted) as individual tests in progress reports and in testdox output. Ref: https://phpunit.readthedocs.io/en/stable/writing-tests-for-phpunit.html#data-providers
This adds extra tests for: * Double quoted strings with interpolated variables. * Nowdoc * Heredocs with and without interpolated variables. Note: for the nowdoc and heredoc tests, the test input is put in single quotes to prevent "nested here/nowdoc", which could skew the tests. These test cases also need to have a new line after the `;` as otherwise PHP does not tokenize these correctly (and the highlighter is not concerned with fixing incorrect tokenization of PHP itself).
f937e38
to
90b3b9e
Compare
I've rebased this PR to show that the tests will still pass on PHP 5.3 as well. |
Ha! Okay, so they weren't passing. Reason being that the short PHP open tags I would normally add this to the matrix to do a complete test run with and without Instead I will add a commit with a tweak to the test workflow to turn that ini setting on for the PHP 5.3 test run. Agreed ? Ref: https://www.php.net/manual/en/ini.core.php#ini.short-open-tag |
The tests for this PR were failing after support for PHP 5.3 had been added back. Reason being that the short PHP open echo tags `<?=` are only recognized as such with the ini setting `short_open_tag` turned on in PHP 5.3. I would normally add this to the matrix to do a complete test run with and without `short_open_tag`, but as this only affects PHP 5.3, I think that's a little over the top. Instead this commit adds a tweak to the test workflow to turn that ini setting on for the PHP 5.3 test run. Ref: https://www.php.net/manual/en/ini.core.php#ini.short-open-tag
@grogy Any chance for a review of this PR in the near future ? Got a number of other PRs lined up as follow-ups to get test coverage up. |
'Double quoted text string with interpolation [1]' => array( | ||
'original' => <<<'EOL' | ||
<?php | ||
echo "Ahoj $text and more 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.
mixing czech and english data samples
Thank you for this bigger PR. Extending test cases and converting to data providers looks great. |
@grogy Thanks for merging, I will line up the next PR now. |
This is a first PR in a series to improve the tests and test coverage for this package.
The PR will probably be easiest to review by looking at the commits individually.
Notes:
Composer: improve PHPUnit version constraints
... as PHPUnit < 5.7.21 doesn't contain the forward compatibility layer with namespaced classes.
PHPUnit: improve configuration
junit
is used somewhere, generating this report for code coverage is unnecessary.Tests: rename the test class/file to TokenizeTest
Realistically, all tests in the
HighligherTest
file are testing the handling of various token types and only incidentally testing the rest of the logic of theHighlighter
class.With that in mind, I'm proposing to rename the test file to make it more obvious what is being tested. This will then also allow for adding additional new test classes to test the rest of the functionality of the
Highlighter
class.Includes adding minimal documentation and a minor method order tweak.
TokenizeTest: use nowdocs instead of heredocs
For most tests, the text within the heredoc should not be interpreted (interpolated). For those tests, it makes more sense to use nowdocs instead of heredocs.
Similar to single quoted versus double quoted strings, text within nowdocs is not interpolated (variables are not expanded), while within heredocs, they are.
Nowdocs are available since PHP 5.3, so can be safely used within this test suite.
Refs:
TokenizeTest: move "empty" tests to new test setup with data providers
This sets up a new structure for this test class using test methods with data providers.
All other existing tests will be converted to data sets in data providers in follow on commits.
TokenizeTest: add tests for PHP tags
Note that the new test data uses NOWDOC format instead of HEREDOC format.
TokenizeTest: move magic constant test cases to data provider
Note: this removes the looping from the test (which was a bad thing).
Advantages of using data providers compared to using a loop within the test function, are:
This prevents one test failure "hiding" behind another failure.
Using a data provider - especially with named cases like used in this commit - will make the error coming from PHPUnit more descriptive and will make debugging which test case failed easier.
Ref: https://phpunit.readthedocs.io/en/stable/writing-tests-for-phpunit.html#data-providers
TokenizeTest: move miscellaneous token test cases to data provider
TokenizeTest: add dedicated test for a T_STRING type token
TokenizeTest: move comment token test cases to data provider
TokenizeTest: add test cases for multi-line comments
TokenizeTest: move text string test cases to data provider
TokenizeTest: add additional test cases for text string handling
This adds extra tests for:
Note: for the nowdoc and heredoc tests, the test input is put in single quotes to prevent "nested here/nowdoc", which could skew the tests.
These test cases also need to have a new line after the
;
as otherwise PHP does not tokenize these correctly (and the highlighter is not concerned with fixing incorrect tokenization by PHP itself).TokenizeTest: add test for inline HTML
TokenizeTest: move function call test cases to data provider
TokenizeTest: move keyword test case to data provider
TokenizeTest: add additional tests for keywords and operators
🆕 GH Actions: set
short_open_tag
ini setting for PHP 5.3 test runThe tests for this PR were failing after support for PHP 5.3 had been added back.
Reason being that the short PHP open echo tags
<?=
are only recognized as such with the ini settingshort_open_tag
turned on in PHP 5.3.I would normally add this to the matrix to do a complete test run with and without
short_open_tag
, but as this only affects PHP 5.3, I think that's a little over the top.Instead this commit adds a tweak to the test workflow to turn that ini setting on for the PHP 5.3 test run.
Ref: https://www.php.net/manual/en/ini.core.php#ini.short-open-tag