PrintingFunctionsTrait: add tests

[rodrigoprimo]

Description

In preparation for supporting PHPCS 4.0, this PR adds unit tests for the PrintingFunctionsTrait::is_printing_function() and PrintingFunctionsTrait::get_printing_functions() methods.

Suggested changelog entry

N/A

Related issues/external references

Related to: #2272

[jrfnl]

❌ I'm going to block this PR in its current state.

The is_printing_function() can't be considered properly tested unless the get_printing_functions() method is also tested.
So either dedicated tests for the get_printing_functions() method need to be added ahead of adding the tests for the is_printing_function() method; or the is_printing_function() tests need to be expanded to include covering what happens in get_printing_functions() (which I believe would be the wrong choice).

Adding tests only for is_printing_function() without having tests for get_printing_functions() is misleading and incomplete.

---

As a side-note regarding the test setup:

I understand what you are trying to do, but why did you choose this way to do it ?

I mean, why not just use PrintingFunctionsTrait; in the test class ? (which would also give the tests access to the private properties if needed).

[rodrigoprimo]

Thanks for the review, Juliette.

The is_printing_function() can't be considered properly tested unless the get_printing_functions() method is also tested.

Regarding testing get_printing_functions(), I opted to add tests only for is_printing_function() because I wrote those tests with the PHPCS 4.0 changes in mind and thought that tests for get_printing_functions() could be added later. I was trying to add just the tests required for PHPCS 4.0 to keep the scope focused on this update. That said, I agree that get_printing_functions() should also have tests, and I can add them to this PR if this is the approach that you prefer.

I understand what you are trying to do, but why did you choose this way to do it ?
I mean, why not just use PrintingFunctionsTrait; in the test class ? (which would also give the tests access to the private properties if needed).

Regarding the test setup, I opted for the anonymous class approach to keep a clear separation between the test subject and the test class. Using use PrintingFunctionsTrait; directly in the test class means the test class becomes the thing it's testing. For a simple trait like PrintingFunctionsTrait, this is not a big concern, but for a trait like WPDBTrait it starts to feel less clean. I wanted to keep a consistent approach across all trait tests.

I can see that the anonymous class approach adds boilerplate (and in the case of the WPDBTrait, I opted to create a helper class that uses the trait instead of an anonymous class, which adds even more boilerplate). I'm more inclined to favor the separation between the test subject and the test class and keep the current structure. That said, I'm open to switching to use in the test class if you think the simplicity outweighs the separation concern. What is your preference?

[jrfnl]

The is_printing_function() can't be considered properly tested unless the get_printing_functions() method is also tested.

That said, I agree that get_printing_functions() should also have tests, and I can add them to this PR if this is the approach that you prefer.

I already said that this PR is blocked until tests for get_printing_functions() are added. Either in a separate PR or in a separate commit in this PR.

Regarding the test setup, I opted for the anonymous class approach to keep a clear separation between the test subject and the test class. ... I wanted to keep a consistent approach across all trait tests.

Understood. Thanks for sharing your reasoning.

I can see that the anonymous class approach adds boilerplate. ... I'm more inclined to favor the separation between the test subject and the test class and keep the current structure. That said, I'm open to switching to use in the test class if you think the simplicity outweighs the separation concern. What is your preference?

No strong preference. I totally see the value of the separation, but I can also see the value of the simpler approach for a simple trait like this if it would make testing the trait more straight-forward (less jumping through hoops to test what needs testing).

I guess for a first set of tests, I would be pragmatic (whatever works), while when iterating on it after, I would consolidate to a standardized, consistent approach based on what we then know is needed. It's difficult to standardize/make things consistent if it's not clear yet what will be needed across the board.

Hope that input helps.

[rodrigoprimo]

I pushed a new commit adding tests for get_printing_functions() in a separate test file. I also force-pushed the first commit as I did a small fix to one of the test docblocks.

No strong preference. I totally see the value of the separation, but I can also see the value of the simpler approach for a simple trait like this

I kept the anonymous class approach for now. We can revisit this decision in the future, as you suggested.

I made a few choices regarding the test coverage for get_printing_functions() that I share below to hear your opinion:

• I don't think it makes sense to check all 14 default printing functions individually, as that would make the test brittle. Instead, I spot-check one known function and verify the return type and value format.
• For custom functions, I verify they are additive by comparing the array count before and after setting customPrintingFunctions, and that custom entries have a false value.
• I also test that the cached result is invalidated when customPrintingFunctions changes.

I'm unsure if I opted for an overly simplistic approach, but I'm inclined to think it is enough for this somewhat simple method. What do you think?

[jrfnl]

Just reviewed the new (second) commit. Hope this helps.

[jrfnl]

May I suggest reworking this test a little to also check the "total count" of the expected return list ? Might also mean that the test name needs a little rethink.

Might also be worth it to have these assertions have $failure message parameter set as this test has multiple assertions in one test. (here and elsewhere)

[rodrigoprimo]

Thanks for your review, @jrfnl. I added a new commit addressing the two suggestions you made.

[jrfnl]

Left two nitpicks. nothing major.

[jrfnl]

Okay, now the tests for get_*() are in place, I've gone back to reviewing the original commit for is_*() and I believe we are still missing one test - a test where the is_*() function returns true for a custom printing function.

Could you please add that @rodrigoprimo ?

[rodrigoprimo]

That makes sense. Thanks for checking the other test class. I just pushed a commit implementing the change you suggested.

[jrfnl]

Thanks for working on this @rodrigoprimo ! All good from me.

For whomever does the second review: please squash-merge.

[rodrigoprimo]

@dingo-d and @GaryJones, Juliette approved this a while ago. Just checking if either of you is available to do the required second review of this PR? Thanks!