Problem/Motivation

I think it can be great to add some unit tests to the central library for the content type negotiation.

Proposed resolution

Unit tests with PHPUnit.

Remaining tasks

Nothing special here.

User interface changes

No.

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

toin0u’s picture

Status: Active » Needs review
FileSize
3.72 KB

It's my very first contribution to drupal :) Hope I'm respecting the rules of the community.

danielmme’s picture

We are working on the DrupalCon 2015 Latin America

danielmme’s picture

Issue tags: +LatinAmerica2015
Crell’s picture

Title: Unit tests for content type negociation » Unit tests for content type negotiation
+++ i/core/tests/Drupal/Tests/Core/ContentNegociationTest.php
@@ -0,0 +1,117 @@
+    $this->assertSame('drupal_dialog', (new ContentNegotiation)->getContentType($this->request));

This syntax will only work on PHP 5.5, I think? Normally you would want to instantiate the class to be tested separately and then test it. Inlining it into the assertion is unconventional and IMO sloppy.

danielmme’s picture

We have applied the patch but coudn't run the tests.

toin0u’s picture

@Crell: Thank you for your review! The syntax works from php>=5.4 but I see your point. I've made an other patch :)

@danielmme: you should be in the core folder in order to run the tests (screnshot: http://cl.ly/image/0x2V1A1W1Z2a).

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Core/ContentNegotiationTest.php
    @@ -0,0 +1,124 @@
    +    $this->request = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')
    +      ->disableOriginalConstructor()
    +      ->setMethods(['get', 'getAcceptableContentTypes', 'getFormat', 'isXmlHttpRequest'])
    +      ->getMock();
    +
    

    I kinda prefer to not worry about mocking value objects like the request. Its not worth, given that the readablity is bad.

  2. +++ b/core/tests/Drupal/Tests/Core/ContentNegotiationTest.php
    @@ -0,0 +1,124 @@
    +    $this->request->expects($this->once())
    +      ->method('getAcceptableContentTypes')
    +      ->will($this->returnValue(['application/json', 'drupal_dialog', 'text/html']));
    +    $this->request->expects($this->at(2))
    +      ->method('getFormat')
    +      ->with('application/json')
    +      ->will($this->returnValue('json'));
    +    $this->request->expects($this->at(3))
    +      ->method('getFormat')
    +      ->with('drupal_dialog')
    +      ->will($this->returnValue('drupal_dialog'));
    

    This is a classical example of hardcoding the test to the implementation. We should not hardcode in which order the getFormat method is called.

toin0u’s picture

@dawehner Well I see your point but I think it prevents regressions very well IMO. What do you think about this http://cgit.drupalcode.org/drupal/tree/core/tests/Drupal/Tests/Core/UrlT... ?

dawehner’s picture

IMHO the request object is a value object ... not mocking things makes things easier to read.

toin0u’s picture

@dawehner Thanks for taking time to review this. I thought about your comment and I was wondering why you consider the Symfony Request object as a value object. Shouldn't a value object be immutable ? The Request object can be easily modified after its creation. And if you don't want/like to mock the request object, do you prefer to use a real instance of a Request to test ?

I'm looking forward to your explanations :)

dawehner’s picture

Well, yes, from a pure definition a value object is immutable, still I think the request object isn't an object which does something but rather just contains information,
which then is used by other piece of code.

Given that the request object is not swappable, so you also don't code against a specification, the request object is the only implementation of it.

toin0u’s picture

Thank you for your answer. I'll see what I can do to make this patch "mergeable" :)

dawehner’s picture

Thank you!

toin0u’s picture

Hope it's better this time :) What do you think @dawehener ?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

toin0u’s picture

Thanks to you :) It's was really nice to have constructive feedbacks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome work on this, toin0u! Thanks to Crell and dawehner for providing mentorship as well.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 8436720 on 8.0.x
    Issue #2424787 by toin0u, dawehner, Crell: Unit tests for content type...

Status: Fixed » Closed (fixed)

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