Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the failure description of StringContains-based assertions when the strings are encoded differently #5505

Conversation

two-plus-two-equals-five
Copy link
Contributor

This PR is a proposed improvement to the existing failure message being generated by the StringContains Constraint based on the discussions about it in the #5128 thread.

For those not familiar with the issue, this improvement specifically adds additional information (the detected string encoding + string length) to the failure message returned for debugging purposes.
Reason for doing so: the console outputs tend to hide the visual difference between two strings when there are different but similar encoding characters (i.e. Unicode's 'thin spaces' verses ASCII's "regular" spaces <-- more on this in the new tests written below).

Example of test output after changes:

Failed asserting that 'SANTAGATI SALVATORE' [ASCII](length: 19) contains "SANTAGATI SALVATORE" [UTF-8](length: 21).

[Stack trace here]

Example of test output before changes:

Failed asserting that 'SANTAGATI SALVATORE' contains "SANTAGATI SALVATORE".

[Stack trace here]

Here is the test that generated this output (note that the 'needle' string uses a thin space character for the space between the two words, whereas the haystack uses a regular space instead):

public function testCanReproduceThinSpaceExample(): void
{
    $this->assertStringContainsString('SANTAGATI SALVATORE', 'SANTAGATI SALVATORE');
}

 

Notes to reviewer:

    ⚛ - this PR was (primarily) written atomically, so reviewing each commit chronologically is recommended for the best review experience

    📝 - this PR's commits contained details of the reasonings for technical decisions made - so if you find yourself thinking: "why did they do that?", please read the commit messages 😊
    🐤 - this is my first PR for PHPUnit so I very much welcome advice / steering on conventions etc

@two-plus-two-equals-five two-plus-two-equals-five marked this pull request as ready for review September 9, 2023 21:52
@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/assertion Issues related to assertions and expectations labels Sep 10, 2023
@sebastianbergmann
Copy link
Owner

Thank you for your contribution. I appreciate the time you invested in preparing this pull request so far. However, this is not yet ready to be merged.

Please fix the failing the tests.

Please follow the workflow for pull requests, specifically:

Please make sure you have set up your username and email address for use with Git. Strings such as silly nick name <root@localhost> look really stupid in the commit history of a project.

@two-plus-two-equals-five
Copy link
Contributor Author

two-plus-two-equals-five commented Sep 10, 2023

Hey @sebastianbergmann, thanks for authorising the workflow run and the kind message.

Odd, those tests passed for me locally on PHP 8.2; I'll see if I can reproduce the issue now.

Are tests passing on one machine and not another common with this pipeline?

Re:

Please make sure you have set up your username and email address for use with Git. Strings such as silly nick name root@localhost look really stupid in the commit history of a project.

I specifically chose my commit name to be two-plus-two-equals-five and email to be the one autogenerated by Github as it obscures my real one after reading that in the CONTRIBUTING.md file.
Please say if this is going to be an issue?

I'll also look at updating the PHPUnit documentation if this PR gets merged (although, on initial viewing, it looks like the assertion outputs are autogenerated by a script on/near release - which is cool).

Thanks again

@two-plus-two-equals-five
Copy link
Contributor Author

two-plus-two-equals-five commented Sep 10, 2023

Update:

It looks like it was late-night human error. I've pushed the fix. @sebastianbergmann can you please reauthorise the workflow run?

@codecov
Copy link

codecov bot commented Sep 10, 2023

Codecov Report

Merging #5505 (a848240) into main (a58a6a2) will increase coverage by 0.01%.
The diff coverage is 97.61%.

@@             Coverage Diff              @@
##               main    #5505      +/-   ##
============================================
+ Coverage     87.34%   87.35%   +0.01%     
- Complexity     6327     6335       +8     
============================================
  Files           676      676              
  Lines         20256    20286      +30     
============================================
+ Hits          17692    17721      +29     
- Misses         2564     2565       +1     
Files Changed Coverage Δ
src/Framework/Constraint/String/StringContains.php 98.38% <97.61%> (-1.62%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sebastianbergmann
Copy link
Owner

Please say if this is going to be an issue?

I try to avoid anonymous commits as much as possible. As far as this PR is concerned, I do not think that it will be a problem.

@two-plus-two-equals-five
Copy link
Contributor Author

Thanks @sebastianbergmann, and thank you for rerunning the pipeline.

Is there anything else you need me todo? I'm happy to answer any questions you may have :)

@sebastianbergmann sebastianbergmann added this to the PHPUnit 10.4 milestone Sep 12, 2023
…n#5128. To consider if it should be its own test (as want to write assertions that prove that each string really is the encoding my comments say they - I think it improves readability)
…ake the expected string lines longer. Renamed existing provider to distinguish between the two
…the Assert class level) "needle and haystack" convention as was hard to follow this test class otherwise. Named all provider test cases to make it easier to understand what each one checks. Drove out failureDescription() changes to include detected encoding and length value for both needle and haystack
…ding = mb_detect_order() yet psalm throws an error with that change. I set param to = null instead as that is its default and its comment says the method will call mb_detect_order() under the hood
@sebastianbergmann sebastianbergmann force-pushed the #5128-proposed-failure-message-improvement-to-aid-debugging-encoding-differences branch from 96d90b6 to a848240 Compare September 12, 2023 05:21
@sebastianbergmann sebastianbergmann merged commit 11c8646 into sebastianbergmann:main Sep 12, 2023
30 checks passed
@sebastianbergmann sebastianbergmann changed the title #5128 - Proposed failure message improvement to aid debugging encoding differences with StringContains-based assertions Improve the failure description of StringContains-based assertions when the strings are encoded differently Sep 12, 2023
@two-plus-two-equals-five
Copy link
Contributor Author

Thanks @sebastianbergmann!

@two-plus-two-equals-five two-plus-two-equals-five deleted the #5128-proposed-failure-message-improvement-to-aid-debugging-encoding-differences branch September 12, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/assertion Issues related to assertions and expectations type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants