Harden test coverage for dependency Constraint class

Here are extra test cases

Ok here are even more test cases for support of

1. 1 or more space or none before version numbers and after operators
2. <> in addition to !=
3. ==, = or no operator for equals.

1. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -25,69 +26,139 @@ public function testIsCompatible(Constraint $version_info, $current_version, $re
   +    foreach (['', '=', '=='] as $equal_operator) {
   +      // Stable version.
   +      $stable = ["{$equal_operator}8.x-1.0", '8.x'];
   +      $tests["$equal_operator(=8.x-1.0)-1.0"] = [$stable, '1.0', TRUE];
   +      $tests["$equal_operator(=8.x-1.0)-1.1"] = [$stable, '1.1', FALSE];
   +      $tests["$equal_operator(=8.x-1.0)-0.9"] = [$stable, '0.9', FALSE];
   +    }
   

   I meant to remove this section because of the loops that are end of this method that swap out the equals operator and other strings.

2. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -25,69 +26,139 @@ public function testIsCompatible(Constraint $version_info, $current_version, $re
   +    foreach ($original_tests as $key => $original_test) {
   +      if (strpos($original_test[0][0], ' ') !== FALSE) {
   +        $tests["$key-no-space"] = $original_test;
   +        $tests["$key-no-space"][0][0] = str_replace(' ', '', $original_test[0][0]);
   +
   +        $tests["$key-extra-space"] = $original_test;
   +        $tests["$key-extra-space"][0][0] = str_replace(' ', '     ', $original_test[0][0]);
   +      }
   +    }
   

   I talked with @wim leers about this issue and he pointed out that these loops put logic in the dataprovider which make the test more complicated and harder to read.

   The next patch will remove this will keeping the added test cases. Instead of looping over the existing test cases it surround the test cases in 3 foreach loops, one for each operator or space that has possible variations.

1. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,126 @@ public function providerIsCompatible() {
   +          $tests["($constraints)-1.1"] = [$less, '1.1', FALSE];
   +          $tests["($constraints)-1.1"] = [$less, '1.0', TRUE];
   +          $tests["($constraints)-1.0"] = [new Constraint("<{$space}8.x-1.0", '8.x'), '1.1', FALSE];
   

   The last 2 cases here have the wrong key. So missed a test case.
   Doing the string concatenation for each test case makes this more likely to have copy/paste errors.

   This next patch has all of this is single method so it only happens once.

2. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -97,7 +156,7 @@ public function providerIsCompatible() {
   -    $constraint = new Constraint('<8.x-4.x,>8.x-1.x', '8.x');
   +    $constraint = new Constraint('<{$space}8.x-4.x,$space>{$space}8.x-1.x', '8.x');
   

   Didn't mean to change this. Fixed

1. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,89 @@ public function providerIsCompatible() {
   +          $constraints = "{$equal_operator}{$space}8.x-2.x,{$space}>= 2.4-alpha2";
   ...
   +          $constraints = "{$equal_operator}{$space}8.x-2.0,$space>= 2.4-alpha2";
   ...
   +          $constraints = ">{$space}8.x-4.x,{$space}< 8.x-1.x";
   

   Missed space characters that can be replaced by $space

2. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,89 @@ public function providerIsCompatible() {
   +          $tests += $this->createTestsForVersions($constraints, ['4.0', '3.9', '2.1', '1,9'], FALSE);
   ...
   +          $tests += $this->createTestsForVersions($constraints, ['4.0', '3.9', '2.1', '1,9'], FALSE, '7.x');
   

   '1,9' should be '1.9'

1. So all of these changes are a lot but I do think it is worth it to add much more test coverage.

   But of course it makes it hard to review because how can we be certain that all the original test cases are still covered?

   So I am to add temporary method testTempToProveAllOldCasesCovered() which will do what it's name says. So I am adding back the original version of providerIsCompatible()(renamed to original_providerIsCompatible() which testTempToProveAllOldCasesCovered() will use.

Also to help reviewing here is a no whitespace patch

Also sorry for using the wrong issue numbers in my patches above 😱

Relating the 2 issues mention in the issue summary

@Wim Leers thanks for the review!

1. re #13.1 we can only hope that a core committer uses their power to demand $spaciness 😜
2. Re #13.3
   

   This is also the case in HEAD, but … why does 1.9 not meet the <8.x-4.x,>8.x-1.x constraint?

   1.9 would satisfy <8.x-4.x,>=8.x-1.x

   But >8.x-1.x doesn't include any 1.x versions

   +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -91,6 +91,102 @@ public function providerIsCompatible() {
   +          // Test greater than.
   +          $constraint = ">{$space}8.x-1.x";
   +          $tests += $this->createTestsForVersions($constraint, ['2.0'], TRUE);
   +          $tests += $this->createTestsForVersions($constraint, ['1.1', '0.9'], FALSE);
   

   This test above confirms this

3. re #13.7 Good catch!
   I move this below

   +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -91,6 +91,102 @@ public function providerIsCompatible() {
   +          // Test greater than and less than.
   +          $constraint = "<{$space}8.x-4.x,{$space}>{$space}8.x-1.x";
   +          $tests += $this->createTestsForVersions($constraint, ['4.0', '1.9'], FALSE);
   +          $tests += $this->createTestsForVersions($constraint, ['3.9', '2.1'], TRUE);
   

   To use the constraint we know passes for some cases.

   I also added a test for 9.x

   In addition added a test for <4.x,>1.x which doesn't specify a core version in the constraint to show that 7.x, 8.x, and 9.x will all have the same behavior when the core version is not specified.

4. I had removed testTempToProveAllOldCasesCovered() locally but I see @Wim Leers left it in his patch and I guess it makes sense to leave it in as long there are changes happening to
   the new providerIsCompatible() to certain we still cover all old cases. We can remove it once it gets closer to RTBC.

1. re #16.3 good point! yes we should be testing 9.x. But also this got me thinking, would we write the same test coverage if we were writing this in the 9.x cycle? Having "8.x" everywhere would look weird. The fact is \Drupal\Component\Version\Constraint is major version agnostic. '8.x' is not the code anywhere except comments. This class and test should not have to be changed in 9.x. Also we really don't want any "8.x" logic to ever unintentionally creep into that class. So I added another outside loop
   

   // Test that core compatibility works for different core versions.
   foreach (['8.x', '9.x', '10.x'] as $core_compatibility) {
   

   This should cover us for a while 😜

   I put the test for cover incompatible core_compatibility outside of this loop so we are still covering that.

2. I am uploading to version of the patch and interdiff.
   1 with testTempToProveAllOldCasesCovered() and drupalci.yml changes still in to prove these latest changes still cover all old cases.
   2 without these changes if @Wim Leers or some one else decides to RTBC

Retesting and setting back to RTBC. #19 was on fail because of Drupal\FunctionalJavascriptTests\Ajax\DialogTest with

Element ... is not clickable at point (646, 331). Other element would receive the click:

Which is random drupalci test failure

Oh wait there is an extra space nit too. New patch

re #22
@xjm thanks for the review!

1. How is empty-string an equality operator? 🤔
   if no operator then equals is assumed so for every equals testing both operators and no operators. Add comment

   changed to $not_equal_operator

2. using $whitespace
3. Agree this affects the readability. We could go back to the method in #4 where all the test cases are made and then we loop through and change the constraint strings. But this str_replace calls in where we don't have to have them. Also it require changing \Drupal\Tests\Component\Version\ConstraintTest::testIsCompatible() to receive strings instead of instance of \Drupal\Component\Version\Constraint

   I personally like the way it is instead because there is less logic and no change to \Drupal\Tests\Component\Version\ConstraintTest::testIsCompatible()

4. I put this outside of the $core_compatibility loop because we have core compatibility in
   +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,102 @@ public function providerIsCompatible() {
   + // Test greater than and less than with an incorrect core
   + // compatibility.
   + $constraint = "<{$space}8.x-4.x,{$space}>{$space}8.x-1.x";
   ...
   + foreach (['8.x', '9.x', '10.x'] as $core_compatibility) {
   

   that shouldn't be replaced with the $core_compatibility.

   But now I changed this to put it inside the loop and just use different constraint string when $core_compatibility === '8.x'
   This covers more branches.

5. I change the descriptions to say where they will ultimately be used.
6. fixed
7. Changed to '::" delimiter

re <a href="#comment-13179942">#22</a>.3 I think if we are going back to using <code> str_replace()

we should use for all operators like in #4 and before

more fixes for #22.5 after chatting with @xjm again

fixed #27

re #31
@Mixologic thanks for the review!

1. re #25.3 : Im -1 on introducing loops and logic, and additional function in the data provider, as not only does it impact readability,

   Yeah was just pointing out we could go this way if the readability was big concern but I would rather not too

2. the whitespace loop is really only proving that \s* in the regex doesnt someday lose its *

   I guess I would say we want a test where the underlying implementation can change completely(to not use regex functions at all say) and if the test still pass we can be confident we aren't breaking BC or any current sites.
   The reason I added the whitespace variation was because I noticed the current test just had 1 constraint string where we were using a space after the operator this wasn't commented on. So we weren't explicitly testing this. If someone had updated that test line for some other reason they could have easily removed the space and it could have gone unnoticed. Poof we have lost test coverage for any space at all after the operator.

   We already currently don't have test coverage for a space after the comma.

   I will upload patch that shows how changing that 1 line would allow the test to pass without support a space after the operator by also removing it from the regex. It will also remove the `\s*` completely for after the comma and show we currently have no test coverage for this.

3. the core compatibility portion of the Constraint class exists only so that we can strip it out, as its not part of the version comparision logic at all.

   Even though we just strip it also effectively makes sure there is not a different major version
   As in this case where only the last test case would fail

   return [
         [new Constraint('8.x-2.0', '8.x'), '2.0', TRUE],
         [new Constraint('2.0', '8.x'), '2.0', TRUE],
         [new Constraint('9.x-2.0', '9.x'), '2.0', TRUE],
         [new Constraint('9.x-2.0', '8.x'), '2.0', FALSE],
       ];
   

   The test coverage also proves it can be provided but it is not necessary.

   So, we're kind of adding coverage for something (core_compatibility) that I don't believe we really want to see in the future. If our goal is to have modules that work with both 8 and 9, then we definitely never want to see '9.x-x.y' in the contrib ecosystem.

   But this how the class works now. There is no exclusion for `9.x` and there is not special logic for `8.x`. If we want this class to exclude 9.x always we should add that class not exclude it from test coverage. adding the test coverage to the class I think just makes it more clear that we have make changes here to exclude 9.x.

4. As per a suggestion from @xjm I added comments with an example before each setting of $constraint to readability. I hope this lets us leave the loops in for the edge cases even though they make actual string less readable.

   We can know what is used in contrib modules for the version constraint but we have no visibility into what people are using in their custom modules. This plus that fact that our Drupal 8 documentation for .info.yml files gives virtually no direction on what can be used for dependency constraints makes important that our test coverage be as robust as possible. This is all it says

   Dependencies can also include version restrictions, for example webform:webform (>=8.x-5.x).

   Our Drupal 7 docs are better but I won't expect someone new to Drupal as of 8 to actually look at these.

re #33 @Mixologic thanks for pushing to have the test more readable and maintainable!

1. It's not easy for me to understand why we have the equality operators in one loop, the inequality operators in another loop, and the greater than/less than/gte/lte operators embedded,

   From my point of view these having the loops nested gives us more test coverage. for instance we have cases for both not equal operators against each equal operator against each core version. If there was some combo that any change made in the future would break this test would break.

2. for example. (i.e when we add ^/~ operators, where would those go? )

   Are we planning on adding those? I would hope we would just wait until #3005229: Provide optional support for using composer.json for dependency metadata to add those operators and at point we would simply be relying on \Composer\Semver\Semver so we wouldn't actually need to add this logic to \Drupal\Component\Version\Constraint so won't have to worry about this test class becoming more complicated.

3. Ultimately those suggestions were a result of me searching for ways to get rid of the loops, and trying to justify not covering those cases in order to do so.

   Ok this patch gets rid of the $whitespace loop and adds 3 cases to the top of the method that test the possible combinations of whitespace that we could have. I think this makes the other constraints for readable because we had $whitespace in every constraint string, usually multiple times.

   I would hope we could leave the other loops because of reasons in state in 1) in this comment.

4. Primarily because I cant easily look at the test and figure out, without unrolling the loops in my head, what it is that we're testing,

   Hope the comments I added in #32

   +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -47,84 +48,88 @@ public function providerIsCompatible() {
   -            // RC version.
   +            // RC version. For example "=8.x-1.1-rc11".
                $constraint = "{$equal_operator}{$whitespace}$core_compatibility-1.1-rc11";
   

   Make this easier to read because you have version of the constraint in the comment without the variables.

pretty sure #37 is unrelated(random?) test failure

1. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,129 @@ public function providerIsCompatible() {
   +          // Test 2 equals with 1 that matching and with nonsensical missing a
   +          // dash. For example "2.x,2.4-beta3".
   +          $constraint = "{$equal_operator} {$core_compatibility}2.x, {$equal_operator} 2.4-beta3";
   +          $tests += $this->createTestsForVersions($constraint, ['2.4-beta3'], FALSE, $core_compatibility);
   

   This comment is wrong updating to match the test case.

2. +++ b/core/tests/Drupal/Tests/Component/Version/ConstraintTest.php
   @@ -27,67 +27,129 @@ public function providerIsCompatible() {
   +          $constraint = "{$equal_operator} {$core_compatibility}2";
   +          $tests += $this->createTestsForVersions($constraint, [$core_compatibility], TRUE, $core_compatibility);
   

   This seems like it should actually not pass.

   I think I got this case from \Drupal\Tests\system\Functional\Module\VersionTest::testModuleVersions() which actually uses "=8.x2.x"

   But it seems like the current case should actually be incompatible.

   I created #3071150: Dependency version constraints evaluate true even the constraint strings aren't valid to fix this. If we think we should not fix because it would break sites then we could keep the test case.
   Or we could it add them in that issue.

🎉 @larowlan thanks for committing and @Wim Leers @xjm and @Mixologic thanks for all the help!