Problem/Motivation

ResourceTestBase uses assertSame for asserting values. This is breaks comparisons of stringified floats, making the test class hard to use.

assertEquals also returns a generated difference between the values, which assertSame does not. This would improve the DX for comparing JSON:API documents.

See #3128322: Filtering variations over JSON:API is always access false.

Steps to reproduce

Use a decimal field with precision storage of 6. 4.00 will serialize to JSON as 4.0 but return from MySQL as 4.00000

Proposed resolution

Switch from assertSame to assertEquals

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 resourcetestbase_sho-3163853-2.patch2.94 KBmglaman
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mglaman created an issue. See original summary.

mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Issue summary: View changes
mglaman’s picture

mglaman
Primary language English
Location WI, USA
CreditAttribution: mglaman at Centarro commented
Title: ResourceTestBase should use assertEquals over assertSame » ResourceTestBase should use assertEquals over assertSame when comparing data
Status: Active » Needs review
FileSize
resourcetestbase_sho-3163853-2.patch2.94 KB

Retitling. It does not need to be changed everywhere. Just whenever it is comparing values from normalized field values.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Itty Bitty Byte commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Thanks for the issue and patch. Marking RTBC since:

1) Patch applies cleanly to 9.2.

2) Tests pass.

3) Additional tests aren't needed since these are tests :)

4) Issue summary and example are clear.

  • catch committed a6258c8 on 9.2.x 
    Issue #3163853 by mglaman, Kristen Pol: ResourceTestBase should use...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed a6258c8 and pushed to 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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