Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#14 | core-phpunit-content-negotiation-2424787-14.patch | 3.28 KB | toin0u |
Comments
Comment #1
toin0u CreditAttribution: toin0u commentedIt's my very first contribution to drupal :) Hope I'm respecting the rules of the community.
Comment #2
danielmme CreditAttribution: danielmme commentedWe are working on the DrupalCon 2015 Latin America
Comment #3
danielmme CreditAttribution: danielmme commentedComment #4
Crell CreditAttribution: Crell commentedThis 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.
Comment #5
danielmme CreditAttribution: danielmme commentedWe have applied the patch but coudn't run the tests.
Comment #6
toin0u CreditAttribution: toin0u commented@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).
Comment #7
dawehnerI kinda prefer to not worry about mocking value objects like the request. Its not worth, given that the readablity is bad.
This is a classical example of hardcoding the test to the implementation. We should not hardcode in which order the getFormat method is called.
Comment #8
toin0u CreditAttribution: toin0u commented@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... ?
Comment #9
dawehnerIMHO the request object is a value object ... not mocking things makes things easier to read.
Comment #10
toin0u CreditAttribution: toin0u commented@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 :)
Comment #11
dawehnerWell, 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.
Comment #12
toin0u CreditAttribution: toin0u commentedThank you for your answer. I'll see what I can do to make this patch "mergeable" :)
Comment #13
dawehnerThank you!
Comment #14
toin0u CreditAttribution: toin0u commentedHope it's better this time :) What do you think @dawehener ?
Comment #15
dawehnerThank you!
Comment #16
toin0u CreditAttribution: toin0u commentedThanks to you :) It's was really nice to have constructive feedbacks!
Comment #17
webchickAwesome work on this, toin0u! Thanks to Crell and dawehner for providing mentorship as well.
Committed and pushed to 8.0.x. Thanks!