Problem/Motivation

Noticed by @alexpott in #2340667-45: Protect Drupal\Core\Language\Language::id, and use getId()

+++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
@@ -70,8 +70,10 @@ public function testGetInfo() {
-    $language = $this->getMock('\Drupal\Core\Language\Language');
-    $language->id = $this->randomMachineName();
+    $values = array('id' => $this->randomMachineName());
+    $language = $this->getMockBuilder('\Drupal\Core\Language\Language')
+      ->setConstructorArgs(array($values))
+      ->getMock();
This is not a real mock :) Since we don't actually mock any functions might as well just create a language object. If we want to use mocks then we should mock the interface and the getId() method. However this is out-of-scope for this change and should be dealt with in a new issue.

Proposed resolution

mock the interface and the getId() method

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch (novice) Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

No.

API changes

No.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

Issue summary: View changes
Xano’s picture

Issue tags: +PHPUnit
YesCT’s picture

Issue summary: View changes
lhangea’s picture

Status: Active » Needs review
FileSize
808 bytes

Patch by cheops90

ebeyrent’s picture

FileSize
2.22 KB
ebeyrent’s picture

FileSize
2.1 KB

Fixed a duplicate expects()

The last submitted patch, 5: 2355543.patch, failed testing.

Status: Needs review » Needs work

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

ebeyrent’s picture

FileSize
2.04 KB

Fixed mock declaration

YesCT’s picture

@cheops90 Did you have trouble getting logged into your drupal.org account?

@ebeyrent Thanks for taking a look at this issue.

Please add a comment explaining the different approach you took from #4

Also, note that interdiffs are useful. For instructions on creating an interdiff, see https://drupal.org/documentation/git/interdiff

martin107’s picture

Status: Needs work » Needs review

Hello ebeyrent, hope you don't mind... I just want to see what the testbot has to say about your patch
so I am bumping the status to "Needs review"

ebeyrent’s picture

I took a slightly different approach for two reasons.

First, while there's only one test method currently, there's bound to be others in the future, so having a fixture seemed more appropriate to me than creating the language interface mock directly in the test method.

The second reason is that it follows the same convention already in place in the unit test - there are already several other mocks that are created in the setUp(), and so it made sense to follow that approach.

What do you guys think?

skipyT’s picture

@YesCT: cheops90 had issues with uploading the patch, cause he doesn't has the not a spammer role yet. he applied for one and lhangea uploaded the patch file instead of him.

YesCT’s picture

@skipyT OK. Please link to the webmasters issue requesting the not a spammer role for @cheops90
[edit: I ask because I did not find it in https://www.drupal.org/project/issues/webmasters ]

Also, you are all welcome in #drupal-contribute on irc.freenode.net https://www.drupal.org/irc

cheops90’s picture

I think the @ebeyrent patch have almost the same approach with my patch, but his advantage how already he said is to use the same LanguageInterface mock in future methods.
So i think we should go with @ebeyrent patch.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

#9 looks good. This is allowed under beta phase because it cleans up internal unit test code.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c1bc910 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation for to the issue summary.

  • alexpott committed c1bc910 on 8.0.x
    Issue #2355543 by ebeyrent, lhangea: TokenTest Language mock is not...

Status: Fixed » Closed (fixed)

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