callstack / reassure

Performance testing companion for React and React Native
https://callstack.github.io/reassure/
MIT License
1.21k stars 25 forks source link

[BUG] Not getting a "Significant Changes to Duration" reported #495

Closed jkoutavas closed 2 weeks ago

jkoutavas commented 1 month ago

Describe the bug I was expecting the output.md file to report a "Significant Changes to Duration" for a <Card> component I purposefully slowed down after doing a baseline on it.

To Reproduce Steps to reproduce the behavior:

Go to any component under test, and add this evil bit of code...

  for (let i = 0; i < 100000; i++) {
    console.log('Delay execution no.' + i);
  }

Then run yarn reassure and look at the output.md. In my case, the <Card> component jumped up dramatically when I added the evil code:

image

...

image

Where evil was added:

export const Card: FC<CardProps> = ({ headerLeft, ...props }) => {
  const theme = useTheme();

  for (let i = 0; i < 100000; i++) {
    console.log('Delay execution no.' + i);
  }

  return (
    <View style={[styles.container, theme.viewStyles.card, props.style]} testID={props.testID}>
    ...

Expected behavior

I would expect Card to be mentioned here (and it isn't)

image

Additional context

My Card.perf-test.tsx:

import React from 'react';
import { View } from 'react-native';
import { measureRenders } from 'reassure';
import { Card } from '.';
import { ThemedIcon } from '../Icon';
import { Text } from '../Text';

test('Card component', async () => {
  await measureRenders(
    <Card
      headerText={<Text.TitleL>Header</Text.TitleL>}
      headerLeft={
        <View>
          <ThemedIcon name="Arrow-left" />
        </View>
      }
      headerRight={
        <View>
          <ThemedIcon name="Arrow-right" />
        </View>
      }
    >
      <Text.BodyL>A card"</Text.BodyL>
    </Card>
  );
});

I'm running this with my RN 0.72.10 app with yarn 3.6.4, Reassure 1.0.0-rc.4 (introduces yarn 3.x and 4.x support), and using Jest ^29.7.0 with "@testing-library/react-native ^12.5.0. I'm using all of Reassure's defaults.

The idea behind this "evil" is to see what the report looks like when a component has degraded in performance. All part of evaluating introduction of Reassure to our project.

mdjastrzebski commented 1 month ago

Hi @jkoutavas, I've done a simple repro in our test app by adding your slow down code and it worked correctly. I had to reduce the loop count from 100 000 to 100, as otherwise the test was running too long for my patience:

  for (let i = 0; i < 100; i++) {
    console.log('Delay execution no.' + i);
  }
image

I think you might have something that optimizes your code in a way that removes console.log calls. Do you see your 'Delay execution no. n' messages in the logs?

Could you either post a simplified example so I can re-create your issue (you can use one of our examples as a quick setup).

mdjastrzebski commented 1 month ago

@jkoutavas is this issue still valid at your end?

jkoutavas commented 1 month ago

@jkoutavas is this issue still valid at your end?

@mdjastrzebski Hi, thanks for asking.

We don't strip console.log() from our app except when built for production:

From our babel.config.js

  if (process.env.NODE_ENV === 'production' || process.env.BABEL_ENV === 'production') {
    plugins.push('transform-remove-console');
  }

Even with this removed (unnecessary, right?) the issue I reported still remains.

Perhaps there's another way to force a slowdown instead of the brute-force "evil" means of doing lots of logging? How do you test for slowdowns?

jkoutavas commented 2 weeks ago

@mdjastrzebski ping :-)

mdjastrzebski commented 2 weeks ago

As written before, it works on my end, with event 100 console.logs considerably slowing the test. You could think of alternative slowing measure, like calculating ~30-th fibonacci number in an ineffective recursive way:

function fib(n: number): number {
  if (n <= 1) {
    return n;
  }

  return fib(n - 1) + fib(n - 2);
}