Wrong message being asserted against. Not sure why this is not being caught when running all the tests. Ah because submodules can not have unit tests.

This is a major bug because we have test coverage that is not being run.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Berdir’s picture

Looks like something else crept into the patch :)

The last submitted patch, 2: 2548061.2.test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2548061.2.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
711 bytes
14.68 KB

Fixing the error.

hussainweb’s picture

Also, as pointed out by @Berdir in #3, there seems to be a lot of other things in the patch. After studying the patch, it seems to me that this (attached) is the only change required to bring the class into test. Please point out if this is wrong.

dawehner’s picture

This doesn't make sense IMHO ... we are testing the test implementation, not something else ... so moving that test is kinda wrong, if you ask me.

Crell’s picture

Component: system.module » routing system

Why are we testing a test implementation of a matcher...? Insert Yo Dawg meme here...

I skimmed through #6 and I'm not entirely sure what we're doing here.

Also moving to the routing system, as it's about matcher coverage.

Crell’s picture

Ah! Wait, now I see. This is the code we had to verify that after the switch to GET parameter format detection it would still be possible for contrib to re-add Accept-header based matching. That's why we have a Yo Dawg situation. :-)

So then, the problem here is JUST that the test is getting skipped, because testbot? Why was this picked up in PHP 7 testing, then?

stefan.r’s picture

I think the problem is just the string, the dot disappeared in PHP7

Berdir’s picture

It's not related to PHP7 at all, I just found it when running unit tests (which I always do with phunit core/modules, which *does* pick this one up).

The expected exception message simply doesn't match the actual exception and we didn't notice because testbot doesn't run the test.

alexpott’s picture

@Berdir it's not just testbot... go to core/ and run phpunit from there. This picks up phpunit.xml.dist and does not run the test either. It a known bug / feature.

Crell’s picture

OK, so side effect of a known issue. Then do we:

1) Move the test so that it gets run (and apparently seems to pass per #7)
2) Postpone on sorting out the known issue?

dawehner’s picture

Well, its not the worst to do this here, but yeah

  * Contains \Drupal\Tests\accept_header_routing_teste\Unit\Routing\AcceptHeaderMatcherTest.

needs to be changed as well.

tim.plunkett’s picture

Issue tags: +rc eligible
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -rc eligible

The last submitted patch, 6: 2548061-6.patch, failed testing.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Oh, missed #15.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
1.39 KB

System module is the wrong place for unit tests, we should not add any there.

Moved to core/tests.

#2859538: system/tests/modules/accept_header_routing_test/tests are in the wrong place, never get executed was marked as duplicate of this.

Wim Leers’s picture

Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 97d9a7b to 8.4.x and 9f1d3c4 to 8.3.x. Thanks!

Backported to 8.3.x cause this is a test only.

removed unused use...

diff --git a/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
index 011ae80..9b2e277 100644
--- a/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
+++ b/core/tests/Drupal/Tests/Core/Routing/AcceptHeaderMatcherTest.php
@@ -3,7 +3,6 @@
 namespace Drupal\Tests\Core\Routing;
 
 use Drupal\accept_header_routing_test\Routing\AcceptHeaderMatcher;
-use Drupal\Tests\Core\Routing\RoutingFixtures;
 use Drupal\Tests\UnitTestCase;
 use Symfony\Component\HttpFoundation\Request;
 

  • alexpott committed 97d9a7b on 8.4.x
    Issue #2548061 by hussainweb, alexpott, klausi, Crell, dawehner, Berdir...

  • alexpott committed 9f1d3c4 on 8.3.x
    Issue #2548061 by hussainweb, alexpott, klausi, Crell, dawehner, Berdir...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.