ContextHelper::is_in_function_call(): add basic tests

[rodrigoprimo]

Description

As part of the work to prepare WPCS for PHPCS 4.0, this PR adds basic tests for ContextHelper::is_in_function_call(). Those tests should help safeguard this method against regressions when updating it to be compatible with PHPCS 4.0.

Since there were no previous tests for utility methods, it was also necessary to modify the test structure to enable running utility tests. They are different from the sniff tests as they extend UtilityMethodTestCase and don't need to run via ./vendor/squizlabs/php_codesniffer/tests/AllTests.php.

After this one is approved, I plan to open another PR, adding tests for more utility methods. I opted to open this one first with tests for a single method to get feedback on the way that I organized the tests.

[jrfnl]

While I understand the desire to have separate tests for this particular utility function, this PR disregards the fact that the method is already being tested.

Additionally:

• I have a feeling that the separate test suite is unnecessary.
• I find the placement of the tests completely illogical.
  I can understand the tests being placed in a WordPress/Tests/Helpers/... subdirectory or a /WordPress/Helpers/Tests/... subdirectory, but I do not understand why a new WordPress/Util/Tests directory is being created with only these tests.
• I find the test markers used, i.e. /* test return false 4 */ very non-descript and unhelpful. These tests are not self-describing.
• Same for the data providers in the test class, which doesn't make things any better.

[jrfnl]

Here and below:

1. Please fix the indent.
2. This may not work as you expect. I've run into trouble with multi-command composer scripts before.

[rodrigoprimo]

Please fix the indent.

Done

This may not work as you expect. I've run into trouble with multi-command composer scripts before.

Do you happen to remember more details, or what you used instead? This seems to be working as I expect. Composer is running both PHPUnit commands. It will not run the second command if the first one fails, but I guess that is not a problem, or is this what you are referring to for some reason that I'm missing?

Running the tests in two separate commands is creating problems for the code coverage report. I'm looking into this now, and I will continue this part of the conversation when I reply to your other remark about creating two separate test suites.

[rodrigoprimo]

Thanks for the review, @jrfnl!

While I understand the desire to have separate tests for this particular utility function, this PR disregards the fact that the method is already being tested.

I assume you are referring to the test for this function in the NonceVerification tests (WordPress/Tests/Security/NonceVerificationUnitTest.1.inc#L294-L305), right? I went ahead and added a new commit to this PR removing those tests, or you had something else in mind?

I have a feeling that the separate test suite is unnecessary.

I believe we explored this a bit together in one of our calls, and were not able to find a way to run the utils tests together with the sniff tests in a single PHPUnit command. Do you have a suggestion on how to do that?

I find the placement of the tests completely illogical.
I can understand the tests being placed in a WordPress/Tests/Helpers/... subdirectory or a /WordPress/Helpers/Tests/... subdirectory, but I do not understand why a new WordPress/Util/Tests directory is being created with only these tests.

Yeah, I'm not happy with this directory as well. To organize the tests for the ContextHelper::is_in_function_call() method and a few of the other utility methods, I used the utility methods tests on PHPCompatibility as a reference. In that repository, those tests are located in PHPCompatibility/Util/Tests, and this led me to create WordPress/Util/Tests. I thought the idea was to separate the sniff and util tests.

Moving the tests to either WordPress/Tests/Helpers/ or WordPress/Helpers/Tests/ makes sense to me. I think I prefer WordPress/Tests/Helpers/, but that could become a problem in the future if we create a sniff category called Helpers.

I find the test markers used, i.e. /* test return false 4 */ very non-descript and unhelpful. These tests are not self-describing.
Same for the data providers in the test class, which doesn't make things any better.

Similar to the above, I tried to follow the structure used for the util tests in PHPCompatibility. I agree that the markers can be more descriptive. Your suggestion is that the test marker describes what is being tested? So for example, /* test return false 1 */ becomes something like /* test not in function call - plain assignment */?

[jrfnl]

I believe we explored this a bit together in one of our calls, and were not able to find a way to run the utils tests together with the sniff tests in a single PHPUnit command. Do you have a suggestion on how to do that?

@rodrigoprimo Yes, we discussed this in a call, no we did not explore. I suggested a number of avenues for you to investigate and you clearly haven't.
Yes, I have a suggestion, several even, have a look at your notes.

[rodrigoprimo]

@jrfnl, I'm sorry, but I've checked my notes from our most recent calls and couldn't find anything about this. I remember that we discussed it, and I recall trying a few things after our conversation without success. Since it has been a while for a couple of different reasons, I don't recall the details anymore.