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

Helpers\IsUnitTestTrait: bug fix - allow for custom test classes passed as FQN #2267

Merged
merged 1 commit into from
Jul 4, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jun 27, 2023

While looking at an old PR (#1960), I realized that custom test classes passed as FQN were not handled correctly by the trait. This code was not adjusted in that PR, just moved, so this bug has existed for a while.

The problem was in this code snippet:

		$known_test_classes = RulesetPropertyHelper::merge_custom_array(
			$this->custom_test_classes,
			$this->known_test_classes
		);

		/*
		 * Show some tolerance for user input.
		 * The custom test class names should be passed as FQN without a prefixing `\`.
		 */
		foreach ( $known_test_classes as $k => $v ) {
			$known_test_classes[ $k ] = ltrim( $v, '\\' );
		}

After the merge of the custom classes, the $known_test_classes array is in string => bool format, so the foreach() loop is absolutely not having the intended effect. The only thing it does, is change the boolean value to strings...

I also noticed that the array merge was being done every single time the is_test_class() method was being called. We can make that a bit more efficient as it will be rare that the list of custom tests classes changes during a run.

So, I have made the following changes:

  • Introduce a new get_all_test_classes() method which will handle the cleaning of the custom names and the merge. This new class uses two new properties $added_custom_test_classes and $all_test_classes to prevent having to do the same merge over and over again.
  • The $known_test_classes property has now been made private.
  • The logic to trim namespace separators of custom test class names has been moved to before the merge as we don't need to trim the names from the $known_test_classes.
  • The logic to trim namespace separators will now work correctly as the value of the $custom_test_classes will be string[] and hasn't been "flipped" yet (keys/values reversed).

Includes additional unit tests.

…ed as FQN

While looking at an old PR (1960), I realized that custom test classes passed as FQN were not handled correctly by the trait.
This code was not adjusted in that PR, just moved, so this bug has existed for a while.

The problem was in this code snippet:
```php
		$known_test_classes = RulesetPropertyHelper::merge_custom_array(
			$this->custom_test_classes,
			$this->known_test_classes
		);

		/*
		 * Show some tolerance for user input.
		 * The custom test class names should be passed as FQN without a prefixing `\`.
		 */
		foreach ( $known_test_classes as $k => $v ) {
			$known_test_classes[ $k ] = ltrim( $v, '\\' );
		}
```

After the merge of the custom classes, the `$known_test_classes` array is in `string` => `bool` format, so the `foreach()` loop is absolutely not having the intended effect. The only thing it does, is change the boolean value to strings...

I also noticed that the array merge was being done every single time the `is_test_class()` method was being called. We can make that a bit more efficient as it will be rare that the list of custom tests classes changes during a run.

So, I have made the following changes:
* Introduce a new `get_all_test_classes()` method which will handle the cleaning of the custom names and the merge.
    This new class uses two new properties `$added_custom_test_classes` and `$all_test_classes` to prevent having to do the same merge over and over again.
* The `$known_test_classes` property has now been made `private`.
* The logic to trim namespace separators of custom test class names has been moved to before the merge as we don't need to trim the names from the `$known_test_classes`.
* The logic to trim namespace separators will now work correctly as the value of the `$custom_test_classes` will be `string[]` and hasn't been "flipped" yet (keys/values reversed).

Includes additional unit tests.
Copy link
Member

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dingo-d dingo-d merged commit ca8a41a into develop Jul 4, 2023
@dingo-d dingo-d deleted the feature/isunittesttrait-bug-fix-and-performance branch July 4, 2023 06:41
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.

3 participants