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

isReady prop passed to children of SplitTreatments is never true during unit tests #13

Closed
brentcklein opened this issue Jun 29, 2020 · 13 comments

Comments

@brentcklein
Copy link

brentcklein commented Jun 29, 2020

Summary

During normal rendering in localhost mode, SplitTreatments first passes isReady: false to its children and returns the control treatment, but shortly after returns isReady: true and returns the treatment defined in config.features as expected. But during testing using jest and testing-library/react, isReady is always false.

I've set up a simple repository to reproduce the issue here: https://github.com/brentcklein/split-tests

Environment

yarn: 1.21.1
node: v14.4.0
react: 16.13.1
react-scripts: 3.4.1
jest: 24.9.0
testing-library/react: 9.5.0
splitio-react: 1.1.0

Example code

// App.jsx

import React from 'react';
import './App.css';

import { SplitFactory } from "@splitsoftware/splitio-react";

import ExampleSplitComponent from "./ExampleSplitComponent";

function App() {
  const splitConfig = {
    core: {
      authorizationKey: "localhost",
      key: "default",
    },
    features: {
      [`prod-feature-1`]: "on",
      [`prod-feature-2`]: "off",
    },
  };

  

  return (
    <SplitFactory config={splitConfig}>
      <ExampleSplitComponent splits={["prod-feature-1", "prod-feature-2"]} />
    </SplitFactory>
  );
}

export default App;
// ExampleSplitComponent.jsx

import React from "react";

import { SplitTreatments } from "@splitsoftware/splitio-react";

const ExampleSplitComponent = ({ splits }) => {
  return splits.map(split => {
    return (
      <SplitTreatments names={[split]} key={split}>
        {({ treatments, isReady }) => {
          return isReady && treatments[split].treatment === "on"
            ? <div>Split {split} is on</div>
            : <div>Split {split} is off</div>;
        }}
      </SplitTreatments>
    );
  });
}

export default ExampleSplitComponent;
// ExampleSplitComponent.test.jsx

import React from "react";
import { SplitFactory } from "@splitsoftware/splitio-react";
import ExampleSplitComponent from "./ExampleSplitComponent";
import { render, waitForElement, getByText } from "@testing-library/react";

it("renders the proper output based on the Split treatment", async () => {
  const splitConfig = {
    core: {
      authorizationKey: "localhost",
      key: "default",
    },
    features: {
      [`test-feature-on`]: "on",
      [`test-feature-off`]: "off",
    },
  };

  const { container } = render(
    <SplitFactory config={splitConfig}>
      <ExampleSplitComponent splits={["test-feature-on", "test-feature-off"]} />
    </SplitFactory>
  );

  const [ featureOn, featureOff ] = await waitForElement(
    () => [
      getByText(container, "Split test-feature-on is on"),
      getByText(container, "Split test-feature-off is off"),
    ],
    { container }
  );

  expect(featureOn).toBeTruthy();
  expect(featureOff).toBeTruthy();
});

The splits render correctly when run with yarn start, but running the unit test results in Unable to find an element with the text: Split test-feature-on is on. with the container output being

<div>
  <div>
    Split test-feature-on is off
  </div>
  <div>
    Split test-feature-off is off
  </div>
</div>
@brentcklein brentcklein changed the title isReady prop passed to children of SplitTreatments is never true during unit tests isReady prop passed to children of SplitTreatments is never true during unit tests Jun 29, 2020
@chillaq
Copy link

chillaq commented Jun 29, 2020

Hi Brent, since react SDK only runs in client side, can you add the section below to package.json and see if it fixes the issue:
"jest": {
"browser": true
}

@chillaq
Copy link

chillaq commented Jul 8, 2020

Hi Brent, just following up on this issue, do you still need help?

Thanks
Bilal

@chillaq
Copy link

chillaq commented Aug 4, 2020

Hi Brent, just want to let you know, I can repro this issue and currently we are working on a fix.

Thanks
Bilal

@chillaq
Copy link

chillaq commented Aug 4, 2020

Hi Brent, we have a workaround,
local.test.js.zip

basically we need to refresh the rendering for the wrapper object since the first initial render is the first initial values for the props, see below:

expect(wrapper.html().includes(expectedText)).toBe(false);
expect(wrapper.html().includes(notReadyText)).toBe(true);
wrapper.update();
expect(wrapper.html().includes(expectedText)).toBe(true);
expect(wrapper.html().includes(notReadyText)).toBe(false);

The sample test script is attached, let me know if you have any questions.

Thanks
Bilal

@EmilianoSanchez
Copy link
Contributor

EmilianoSanchez commented Aug 5, 2020

Hello @brentcklein ,

Also, as Bilal suggested here you must set the browser Jest configuration in your package.json, in order to properly run Jest tests with React SDK. This is because React SDK uses JS SDK underneath, which exposes a different API for Node than for Browsers (and Jest uses the Node variant of the API by default).

Since your example project was created via create-react-app (CRA) you must also eject it first with yarn eject because CRA doesn't support overriding the browser config parameter of Jest.

In summary, you should follow these steps in the root of your example https://github.com/brentcklein/split-tests:
1- eject the project (yarn eject) to use the browser jest configuration.
2- add the line "browser": true, in the jest configuration at project package.json.
3- finally, upgrade React SDK to version 1.2.0 (yarn add @splitsoftware/splitio-react@1.2.0) which includes several bug fixes.

After that, yarn test should run OK.

Thanks,
Emiliano

@nunosimoes-tdx
Copy link

I'm having the exact same issue and none of the solutions above worked for me. I don't know if you realized that @brentcklein is using React Testing Library and the provided solution was using enzyme? I'm using RTL too and the isReady property is always false. Any tip on how to solve this?

@chillaq
Copy link

chillaq commented Oct 28, 2020

Hi @nunosimoes-tdx, Did you eject your project and set the test to run on browser mode? This is a required step to get the test to work.

Thanks
Bilal

@nunosimoes-tdx
Copy link

Hi @chillaq, thanks for you reply.
I didn't need to eject the project because I've configured everything from scratch (with no boilerplate).
I've set the "browser": true in jest config on the package.json file.
I'm using jest 26.1.0 with react testing library 10.4.3.
If you want to we can schedule a call to try to figure out what's the issue on using jest with RTL.

@nunosimoes-tdx
Copy link

I see that since 26.0.0 version of jest, the browser flag was deprecated. We tried to use the recommended browser-resolve but we blocked in one issue that was already reported without any feedback (jestjs/jest#10547).

Seems that we need to rethink our strategy on unit testing with react split lib.

@EmilianoSanchez
Copy link
Contributor

EmilianoSanchez commented Feb 11, 2021

hi @brentcklein , @nunosimoes-tdx

It has been a while, but we have finally released v1.2.4 of the React SDK, which includes an update to solve the issues you were facing when using Jest for unit testing.

Now it is not required to configure Jest with neither the browser option or browser-resolve. In other words, you can now use the default Jest configuration, provided, for example, by create-react-app projects.

Please, let us know if you need further details.

@RobertGardner
Copy link

I'm experiencing this problem using v1.2.4 of the React SDK, but with a slight twist. If I have just one test in my test file then it passes (isReady changes to true). However, if I have more than one test, the first one passes but subsequent ones fail (isReady never changes to true).

Because the tests each render <SplitFactory />, the SplitFactory component gets repeatedly mounted and dismounted. In an attempt to avoid recreating the underlying factory, I tried passing the identical instance of config with each use, but that didn't fix it. So I tried creating the factory externally and passing it to SplitFactory. This works if I create just one instance of the factory. But if I dispose the old and create a new factory for each test, then the later tests fail again (isReady never changes to true).

@EmilianoSanchez
Copy link
Contributor

EmilianoSanchez commented Jun 3, 2021

I'm experiencing this problem using v1.2.4 of the React SDK, but with a slight twist. If I have just one test in my test file then it passes (isReady changes to true). However, if I have more than one test, the first one passes but subsequent ones fail (isReady never changes to true).

Because the tests each render <SplitFactory />, the SplitFactory component gets repeatedly mounted and dismounted. In an attempt to avoid recreating the underlying factory, I tried passing the identical instance of config with each use, but that didn't fix it. So I tried creating the factory externally and passing it to SplitFactory. This works if I create just one instance of the factory. But if I dispose the old and create a new factory for each test, then the later tests fail again (isReady never changes to true).

Hi @RobertGardner,

Thank you for your comment. It led us to an issue with the localhost mode in our JS SDK, which is used by the React SDK. Basically, SDK factory instances in localhost mode were all sharing the same reference to the mocked features and events were only emitted for the first instance.
We have fixed that in the new release of both libraries, so that each SDK instance in localhost mode can properly be updated and emit SDK events independently.

For react, check React SDK v1.2.6). It should solve your problem with <SplitFactory /> and localhost mode in multiple UTs.

Thanks,
Emiliano

@RobertGardner
Copy link

@EmilianoSanchez
v1.2.6 fixed the problem I was having. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants