Problem

There are many assertions where the argument order doesn't match the parameters when comparing values with assertSame()

Proposed Solution

Swap the arguments for assertSame() assertions to match the expectation of their parameters.

Order should be expected first and actual second arguments:
assertSame($expected, $actual

CommentFileSizeAuthor
#20 2887161-20-D8.patch51.42 KBmohit1604
#14 2887161-14-D8.patch53.73 KBmohit1604
#11 2887161-11-D8.patch53.73 KBmohit1604

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Most but not all of these swaps will be due to #2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented
Issue tags: +dcco2017

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mayurjadhav’s picture

mayurjadhav commented
Issue tags: +DrupalMumbaiCodeSprint
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented
Issue tags: -dcco2017

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

SherFengChong’s picture

SherFengChong commented
Assigned: Unassigned » SherFengChong

I will work on this

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Assigned: SherFengChong » Unassigned
Issue tags: -DrupalMumbaiCodeSprint

Sher is not going to work on this anymore.

mohit1604’s picture

mohit1604 commented
Assigned: Unassigned » mohit1604
mohit1604’s picture

mohit1604 commented
Status: Active » Needs review
StatusFileSize
new 2887161-11-D8.patch53.73 KB

Providing required patch , Please review it :)

Status: Needs review » Needs work

The last submitted patch, 11: 2887161-11-D8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mohit1604’s picture

mohit1604 commented

Working on fixing test ;)

mohit1604’s picture

mohit1604 commented
Status: Needs work » Needs review
StatusFileSize
new 2887161-14-D8.patch53.73 KB
mohit1604’s picture

mohit1604 commented
Assigned: mohit1604 » Unassigned
Status: Needs review » Needs work
mohit1604’s picture

mohit1604 commented
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

I went through all the changes and every single of them look like the left value is the expected value.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This doesn't apply to 8.5.x, so I've committed it only to 8.6.x.

  • catch committed b211213 on 8.6.x 
    Issue #2887161 by Mohit Malik, joelpittet: Ensure argument order is...
mohit1604’s picture

mohit1604 commented
Status: Fixed » Needs review
StatusFileSize
new 2887161-20-D8.patch51.42 KB

@catch , Thanks for committing to 8.6.x , this patch is for 8.5.x :)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 8.6.x-dev » 8.5.x-dev
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

OK this is worth trying to keep in sync, so moving back to RTBC.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

❤️

dawehner’s picture

dawehner
Primary language German
commented

Too bad it is not possible to provide some check for this automatically! Thakn you @Mohit Malik for this. This makes failed tests easier to understand.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed b79a827 and pushed to 8.5.x. Thanks!

  • catch committed b79a827 on 8.5.x 
    Issue #2887161 by Mohit Malik, joelpittet: Ensure argument order is...

Status: Fixed » Closed (fixed)

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