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

Coverage flag breaks snapshot test #1740

Closed
ColCh opened this issue Sep 19, 2016 · 22 comments · Fixed by #1752
Closed

Coverage flag breaks snapshot test #1740

ColCh opened this issue Sep 19, 2016 · 22 comments · Fixed by #1752
Assignees
Labels
Milestone

Comments

@ColCh
Copy link
Contributor

ColCh commented Sep 19, 2016

I think it's because of code instrumentation

jest --coverage
    - Snapshot
    + Received
...
    -   onSelect={[Function onSelectHandler]}
    +   onSelect={[Function anonymous]}

Running with jest is just OK.

Source code and func name for onSelectHandler:
jest:

    console.log src/components/pie-chart.js:44
      [Function: onSelectHandler]
    console.log src/components/pie-chart.js:45
      function onSelectHandler(_ref2){var nativeEvent=_ref2.nativeEvent;

      if(!onSelect){
      return;
      }
      onSelect(nativeEvent.index);
      }

And for jest --coverage:

    console.log src/components/pie-chart.js:44
      [Function]
    console.log src/components/pie-chart.js:45
      function (_ref2){var nativeEvent=_ref2.nativeEvent;++__cov_742n7agVtRgakP6RGrGnDGRBizg.f['3'];++__cov_742n7agVtRgakP6RGrGnDGRBizg.s['7'];

      if(!onSelect){++__cov_742n7agVtRgakP6RGrGnDGRBizg.b['1'][0];++__cov_742n7agVtRgakP6RGrGnDGRBizg.s['8'];
      return;
      }else{++__cov_742n7agVtRgakP6RGrGnDGRBizg.b['1'][1];}++__cov_742n7agVtRgakP6RGrGnDGRBizg.s['9'];
      onSelect(nativeEvent.index);
      }

How I can avoid this issue? Force use anonymous functions on every place? 😯

@nornagon
Copy link

I've also noticed tests that pass consistently without --coverage break consistently when --coverage is passed. I'm not sure how to inspect the instrumented code, but happy to help out debugging this if I can.

@ColCh
Copy link
Contributor Author

ColCh commented Sep 19, 2016

Degugging:

console.log(onSelectHandler);
console.log(onSelectHandler+'');

First line will show function name, second will show function source code.

I think this issue comes from istanbul, I can investigate it tomorrow

@cpojer
Copy link
Member

cpojer commented Sep 20, 2016

cc @DmitriiAbramov

@ColCh
Copy link
Contributor Author

ColCh commented Sep 20, 2016

Yes, this is istanbul.

Related issue: gotwarlost/istanbul#699

For now, I wanted to point out a hotfix for that. Just wrap your named func into anonymous proxy func:

@@ -44,7 +44,7 @@ const PieChart = ({
   return (
     <RCTPieChart
       chartRotation={chartRotationInRadians}
-      onSelect={onSelectHandler}
+      onSelect={(...args) => onSelectHandler(...args)}
       segments={processedSegments}

And snapshot transforms then to this:

@@ -2,7 +2,7 @@ exports[`PieChart Android should render pie chart 1`] = `
 <RCTPieChart
   chartRotation={-0.2617993877991494}
   innerCircleRadius={0.55}
-  onSelect={[Function onSelectHandler]}
+  onSelect={[Function onSelect]}
   segments={
     Array [
       Object {

However, this is a hack...

@aaronabramov
Copy link
Contributor

yeah.. it seems like there's nothing we can do on the jest side, unless we omit function names in snapshots.
in theory it should be possible for istanbul to keep the names of most of the functions using only static analysis, right?

@ColCh
Copy link
Contributor Author

ColCh commented Sep 20, 2016

@DmitriiAbramov Take a look on my snippets above...

Recap. Given this handler (AFAIK it should be an anonymous function):

 onSelect={(...args) => onSelectHandler(...args)}

It writes this handler in snapshot:

onSelect={[Function onSelect]}

How can it be?

@cpojer cpojer added this to the 16.0.0 milestone Sep 20, 2016
@cpojer cpojer self-assigned this Sep 20, 2016
@cpojer
Copy link
Member

cpojer commented Sep 21, 2016

Yeah we should probably just get rid of function names in the snapshots. I originally liked this a lot but it is causing more trouble than it is worth.

@cpojer
Copy link
Member

cpojer commented Sep 21, 2016

Will be fixed in 16.

@cpojer cpojer closed this as completed Sep 21, 2016
@guigrpa
Copy link
Contributor

guigrpa commented Nov 18, 2016

Hmmm... doesn't solve the problem completely if for some reason you're snapshotting a React element. It's name will turn Unknown in the snapshot when you enable --coverage.

@vincentaudebert
Copy link

vincentaudebert commented Nov 23, 2016

+1 @guigrpa
screen shot 2016-11-23 at 7 21 02 pm
That's what I get after running jest --watch just after running jest --coverage (and having updated my snapshots with coverage)

@dotfold
Copy link
Contributor

dotfold commented Dec 11, 2016

In my case I'm using enzyme to snapshot a functional stateless component in React, and I get a failed snapshot assertion with coverage enabled, but it's not Unknown - for me it renders Component:

-     <ListItem
+     <Component
          active={false}
          item={
            Object {
              "id": "thing",
              "name": "stuff",
            }
          }
          onClick={[Function]} />

I'd be happy to isolate a repro case for this setup if it helps at all. The curious part here is that all my other snapshots are fine, it's just this instance of snapshotting a functional stateless component that has this differing output.

@thymikee
Copy link
Collaborator

@dotfold if you can isolate the test case, please file a new issue with it.

@vincentaudebert
Copy link

If it helps, I get the Unknown on stateless components too. Could give a hint on where to look for.

@vincentaudebert
Copy link

vincentaudebert commented Dec 13, 2016

@thymikee I'm working on this at the moment https://github.com/springload/mocha-chai-to-jest

If you checkout the repo, set it up and run npm run test:jest:coverage you will get the error with the stateless component StatelessAnimal used inside CatBar component.

I quickly tried to use a statefull component like HatchingAnimal inside CatBar and I can confirm it doesn't trigger the error. The HatchingAnimal component is generated properly.

screen shot 2016-12-13 at 4 16 24 pm

Let me know if this helps.

@thymikee
Copy link
Collaborator

For stateless components you need to define displayName prop of the Component. See this thread for more background: #1824 (comment)

@vincentaudebert
Copy link

@thymikee Cool it fixed my problem. I'm just wondering how come the "normal" jest testing finds the name properly and why the coverage doesn't?

Also using displayName property could lead to some typo mistakes, etc. Why not just use the component name? Like what "normal" jest seems to do.

@thymikee
Copy link
Collaborator

yeah this kind of sucks. Istanbul wraps functions with other anonymous functions and we take the function name that node gives to the rendered component :(
~ @cpojer

@cpojer
Copy link
Member

cpojer commented Dec 14, 2016

Node 6 infers function names.

Example:

const Banana = () => {};
console.log(Banana.name); // "Banana"

this wasn't possible in previous versions of node.

It works fine when you aren't using coverage but when you do, here is what istanbul does to your code:

const Banana = (__cov[…], () => {});
console.log(Banana.name) // empty

because of how the expression is defined, node cannot infer the name properly. The only way to fix it is in istanbul itself or by using a babel transform that changes const Banana = () => {} to const Banana = function Banana() {}.

@tomchentw
Copy link

const Banana = (__cov[…], () => {});
console.log(Banana.name) // empty
@cpojer: because of how the expression is defined, node cannot infer the name properly.

Exactly!

I've recently encountered this issue and I tried to fix it on the istanbul side. I forked babel-plugin-istanbul to fix this, and I put my discovery in here:

istanbuljs/babel-plugin-istanbul#125

Could you folks let me know what you think about my proposal?

@CGamesPlay
Copy link

Per istanbuljs/babel-plugin-istanbul#125 (comment) this is now fixed in babel-plugin-istanbul@4.1.5 but requires jest --no-cache in order to work. @cpojer, can this issue be re-opened? Could file a separate one if need by.

@tomchentw
Copy link

tomchentw commented Oct 24, 2017

@CGamesPlay you can use jest@^21.3.0-beta.2 and run yarn jest --clearCache. After this, jest should compile your code with the new istanbul instrument code for no problem (no --no-cache is needed).

@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 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants