Skip to content
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

UtilityMethodTestCase::getTargetToken(): throw a custom exception when target token not found #371

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 17, 2022

PR #248 introduced the option to throw an exception when the target token couldn't be found (instead of failing the test). In that initial implementation, the exception thrown would be the PHP native RuntimeException.

This commit changes the principle:

  • The new parameter $failTest has been removed.
  • A new, PHPCSUtils native PHPCSUtils\Exceptions\TestTargetNotFound exception is introduced, which extends the PHP native OutOfBoundsException.
  • If the target token is not found, the test will now always throw a TestTargetNotFound exception.

As the change from PR #248 has not been in a release yet, the removal of the parameter is non-breaking.

All the same, this is a functional change as an uncaught exception will result in a test being marked as "errored", while previously, the test would be marked as "failed" when the target token could not be found.

The reason for making this change anyway are as follows:

  1. The test run will still be marked as unsuccessful, so in that sense, there is no change.
  2. Marking the test as "errored" is semantically more correct as a basic condition for the test to be able to run (having a token to examine) has not been fulfilled.
  3. If a test would need to look for different token (contents) depending on PHP/PHPCS versions, they can choose to catch the exception and do a second token search. This means that the benefits of the change from UtilityMethodTestCase::getTargetToken(): add option to throw an exception... #248 are still in place.

Includes perfunctory tests for the new TestTargetNotFound exception. Includes updating the related test in the GetTargetTokenTest class to expect the new exception.

…n target token not found

PR 248 introduced the option to throw an exception when the target token couldn't be found (instead of failing the test).
In that initial implementation, the exception thrown would be the PHP native `RuntimeException`.

This commit changes the principle:
* The new parameter `$failTest` has been removed.
* A new, PHPCSUtils native `PHPCSUtils\Exceptions\TestTargetNotFound` exception is introduced, which extends the PHP native `OutOfBoundsException`.
* If the target token is not found, the test will now always throw a `TestTargetNotFound` exception.

As the change from PR 248 has not been in a release yet, the removal of the parameter is non-breaking.

All the same, this is a functional change as an uncaught exception will result in a test being marked as "errored", while previously, the test would be marked as "failed" when the target token could not be found.

The reason for making this change anyway are as follows:
1. The test run will still be marked as unsuccessful, so in that sense, there is no change.
2. Marking the test as "errored" is semantically more correct as a basic condition for the test to be able to run (having a token to examine) has not been fulfilled.
3. If a test would need to look for different token (contents) depending on PHP/PHPCS versions, they can choose to `catch` the exception and do a second token search. This means that the benefits of the change from 248 are still in place.

Includes perfunctory tests for the new `TestTargetNotFound` exception.
Includes updating the related test in the `GetTargetTokenTest` class to expect the new exception.
@jrfnl jrfnl added this to the 1.0.0-alpha4 milestone Oct 17, 2022
@jrfnl jrfnl merged commit 2970e54 into develop Oct 17, 2022
@jrfnl jrfnl deleted the testutils/utilitymethodtestcase-gettargettoken-throw-instead-of-fail branch October 17, 2022 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant