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

Strange tests titles on Node.js v12 #8443

Closed
litichevskiydv opened this issue May 8, 2019 · 7 comments
Closed

Strange tests titles on Node.js v12 #8443

litichevskiydv opened this issue May 8, 2019 · 7 comments

Comments

@litichevskiydv
Copy link

🐛 Bug Report

test.each doesn't use test case toString for tests titles generation on Node.js v12 and produces titles like this:

{ toString: [Function: toString] }

To Reproduce

Situation can be reproduced via executing such test on Node.js v12.

describe("Should not use toString", () => {
  const testCases = [
    {
      toString: () => "Clear test title"
    }
  ];

  test.each(testCases)("%s", testCase => {});
});

Expected behavior

Test title ought to be "Clear test title" as it is on Node.js v8 and v10.

Link to repl or repo (highly encouraged)

Test cases for which strange titles are generated:
https://github.com/litichevskiydv/JsUsefulPrimitives/blob/master/tests/manipulation/manipula.test.js

Build log for Node.js v10:
https://travis-ci.org/litichevskiydv/JsUsefulPrimitives/jobs/529938904
Build log for Node.js v12:
https://travis-ci.org/litichevskiydv/JsUsefulPrimitives/jobs/529938905

Run npx envinfo --preset jest

Paste the results here:

  System:
    OS: Linux 4.15 Ubuntu 16.04.6 LTS (Xenial Xerus)
    CPU: (2) x64 Intel(R) Xeon(R) CPU @ 2.30GHz
  Binaries:
    Node: 12.2.0 - ~/.nvm/versions/node/v12.2.0/bin/node
    Yarn: 1.15.2 - /usr/local/bin/yarn
    npm: 6.9.0 - ~/.nvm/versions/node/v12.2.0/bin/npm
  npmPackages:
    jest: 24.8.0 => 24.8.0 
@mattphillips
Copy link
Contributor

Hmm this is strange, we are using util.format to handle the pretty printing of test titles in printf format.

I'd say this is potentially a bug in Node@12, I've just read over the util.format docs and it doesn't mention anything in regards to objects that override the toString method.

@SimenB @thymikee @jeysal what do you guys think, should this be opened up in the node repo?

@SimenB
Copy link
Member

SimenB commented May 9, 2019

@BridgeAR what do you think, is this a bug? I noticed nothing about it in the release notes for node 12 either

@BridgeAR
Copy link

BridgeAR commented May 9, 2019

This was an intentional change: nodejs/node#26927

I did not expect this change to cause any issues (passing through objects to the util.format %s formatter is mostly a mistake and result in [object Object]). I personally do not think it's a good idea to rely on the toString behavior and I suggest to change the code to a more "direct" way to write the test cases. I'll nevertheless open a PR to check for the presence of toString and only use util.inspect if it's not present on objects.

BridgeAR added a commit to BridgeAR/node that referenced this issue May 20, 2019
This makes sure that `util.format` uses `String` to stringify an object
in case the object has an own property named `toString` with type
`function`. That way objects that do not have such function are still
inspected using `util.inspect` and the old behavior is preserved as
well.

PR-URL: nodejs#27621
Refs: jestjs/jest#8443
Reviewed-By: Roman Reiss <[email protected]>
@BridgeAR
Copy link

I just landed a PR in Node.js that should be included in the next version. It makes sure that %s uses toString functions on objects in case the function is defined by the user.

@SimenB
Copy link
Member

SimenB commented May 20, 2019

Awesome, thanks!

targos pushed a commit to nodejs/node that referenced this issue May 20, 2019
This makes sure that `util.format` uses `String` to stringify an object
in case the object has an own property named `toString` with type
`function`. That way objects that do not have such function are still
inspected using `util.inspect` and the old behavior is preserved as
well.

PR-URL: #27621
Refs: jestjs/jest#8443
Reviewed-By: Roman Reiss <[email protected]>
@litichevskiydv
Copy link
Author

Guys thanks a lot! I've just rerunned tests and titles looks as they were before. I'm closing the issue.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants