[BUG] Default <OptimizelyVariation> always rendered when not final child of <OptimizelyExperiment>

[alexparish]

Is there an existing issue for this?

• [X] I have searched the existing issues

SDK Version

2.9.2

Current Behavior

The <OptimizelyVariation> component with default attribute is rendered in all cases when it appears as the first child component of <OptimizelyExperiment>:

<OptimizelyExperiment experiment="simple_experiment">
  <OptimizelyVariation default>
    default
  </OptimizelyVariation>
  <OptimizelyVariation variation="control">
    control
  </OptimizelyVariation>
  <OptimizelyVariation variation="variation">
    variation
   </OptimizelyVariation>
</OptimizelyExperiment>

Expected Behavior

The correct chosen variation should be rendered. Which variation is rendered which should be agnostic of the position of the default <OptimizelyVariation> component within the <OptimizelyExperiment> component.

Steps To Reproduce

Configure an experiment with the following code:

<OptimizelyExperiment experiment="simple_experiment">
  <OptimizelyVariation default>
    default
  </OptimizelyVariation>
  <OptimizelyVariation variation="control">
    control
  </OptimizelyVariation>
  <OptimizelyVariation variation="variation">
    variation
   </OptimizelyVariation>
</OptimizelyExperiment>

Ensure the visitor is eligible for the experiment and that the control or variation are configured to be randomly chosen as the variation. Enable the experiment.

The <OptimizelyVariation> component with default attribute will always be rendered even when the experiment is enabled and the experiment decision is "control" or "variation".

React Framework

18.2.0

Browsers impacted

All

Link

https://codesandbox.io/p/sandbox/optimizely-demo-forked-rsjkml?file=%2Fsrc%2Findex.jsx

Logs

No response

Severity

Minor issue

Workaround/Solution

No response

Recent Change

No response

Conflicts

No response

[mikechu-optimizely]

Hey @alexparish. Thanks for the report. Let me spin up your scenario.

Just to make sure I understand the story here:

• The OptimizelyVariation with default is always being chosen
• Looking at the dev docs I think I agree with you and this should act like a switch statement sort of.

Can you try for me (perhaps you'll get to it faster):

<OptimizelyExperiment experiment="simple_experiment">
  <OptimizelyVariation variation="control">
    control
  </OptimizelyVariation>
  <OptimizelyVariation variation="variation">
    variation
   </OptimizelyVariation>
  <OptimizelyVariation default>
    default
  </OptimizelyVariation>
</OptimizelyExperiment>

where we've moved the default option to the bottom.

[mikechu-optimizely]

Oh shoot. You said that in the title and current behaviour. Sorry Mate.

Let me read the Issue (better) and SDK source and get back to you.

[mikechu-optimizely]

For reference, I've created an internal ticket FSSDK-9839 for this.

[alexparish]

That's great thanks. I wonder if it's worth tackling https://github.com/optimizely/react-sdk/issues/86 at the same time given they are in the same area of the codebase?

[mikechu-optimizely]

Nice. Agreed.

[mikechu-optimizely]

I threw a quick additional unit test in Experiment.spec.tsx which passed.

it('should render using <OptimizelyVariation default> in first position', async () => {
      const { container } = render(
        <OptimizelyProvider optimizely={optimizelyMock}>
          <OptimizelyExperiment experiment="experiment1">
            <OptimizelyVariation default>
              <span data-testid="variation-key">default variation</span>
            </OptimizelyVariation>
            <OptimizelyVariation variation="otherVariation">
              <span data-testid="variation-key">other variation</span>
            </OptimizelyVariation>
          </OptimizelyExperiment>
        </OptimizelyProvider>
      );

      // while it's waiting for onReady()
      expect(container.innerHTML).toBe('');

      // Simulate client becoming ready
      resolver.resolve({ success: true });

      await optimizelyMock.onReady();

      await waitFor(() => expect(screen.getByTestId('variation-key')).toHaveTextContent('default variation'));
    });

I think the logic's also correct, reading the React.Children.forEach in Experiment.tsx L63-75.

Time for an e2e test scenario.

[mikechu-optimizely]

Nope. That was an incorrect unit test. This test fails as expected.

    it('should NOT render using <OptimizelyVariation default> in first position when matching variation', async () => {
      const { container } = render(
        <OptimizelyProvider optimizely={optimizelyMock}>
          <OptimizelyExperiment experiment="experiment1">
            <OptimizelyVariation default>
              <span data-testid="variation-key">default variation</span>
            </OptimizelyVariation>
            <OptimizelyVariation variation="variationResult">
              <span data-testid="variation-key">matching variation</span>
            </OptimizelyVariation>
          </OptimizelyExperiment>
        </OptimizelyProvider>
      );

      // while it's waiting for onReady()
      expect(container.innerHTML).toBe('');

      // Simulate client becoming ready
      resolver.resolve({ success: true });

      await optimizelyMock.onReady();

      await waitFor(() => expect(screen.getByTestId('variation-key')).toHaveTextContent('matching variation'));
    });

[mikechu-optimizely]

This fix was released with version 3.0.0. We'd love feedback when you have a chance.