undefined | Better HN

0 pointsdrewcoo2y ago0 comments

If you want to do it with the whole page and talk only to the local code, then yes, I'd recommend Sinon. I think that's a much simpler solution than . . . creating an all new NIH framework!

I'd also recommend refactoring to a more mock-friendly way to do that countdown if you don't want to cover up all the internal logic.

If the timeout interval is loaded remotely from some API (and it probably is if you have reasonably configurable nag popups), then you can always mock that API call.

0 comments

kolodny2y ago

The point is that you shouldn't need to rewrite your countdown component to allow testing. Can you provide a snippet of that change and what the test would look like?

Not being toggle parts of the app is the root of the issue when creating e2e tests. For example overriding a feature flag, you could find the API call for the feature but what if it's a grpc call and part of your dev build pulls in any update, you can't easily change the binary response with confidence anymore.

The current solutions are good enough to do smoke tests, but nitty-gritty e2e tests don't scale well.

In the example above it's simple

    export const CountdownTime = createOverride(60);
    export const Countdown = () => {
      const initialTime = CountdownTime.useValue();
      const [time, setTime] = React.useState(initialTime);
      React.useEffect(() => {
        const interval = setInterval(() => {
          setTime(t => t - 1);
          if (t === 1) clearInterval(interval);
        }, 1000);

        return () => clearInterval(interval)
      }, []);

      return time > 0 ? <>Time left {time}</> : <>Done</>
    };

    it('tests faster', async () => {
      const { page } = await render(
        app => <CountdownTime.Override with={() => 5}>{app}</CountdownTime.Override>
      );
      await expect(page.getByText('Done')).not.toBeVisible();
      page.waitForTimeout(5000);
      await expect(page.getByText('Done')).toBeVisible();
    });

If I needed something like this, I'd probably also make setInterval an override as well, so I don't need to wait at all, but you get the idea.

nsfmc2y ago

i think the library's approach to DI is pretty neat (and meets a team where they are which is worth a lot), but i think you're running into an issue where people are saying that instead of working around the realities of your codebase, team and testing needs, you should have done something like this.

  const useCountdownValue = initialTime => {
    const [time, setTime] = React.useState(initialTime);
    React.useEffect(() => {
      const interval = setInterval(() => {
        setTime(t => t - 1);
        if (t === 1) clearInterval(interval);
      }, 1000);
  
      return () => clearInterval(interval)
    }, []);
    return time;
  }

  const Countdown = ({time}: {time: number}) => {
    return time > 0 ? <>Time left {time}</> : <>Done</>
  }
  
  const ActualCountdownInContextSomewhere = () => {
    const remainingSeconds = useCountdownValue(60)
    return <>
      <Countdown time={remainingSeconds} />
    </>
  }

i'll say, i have never written a test for a hook or looked into what's needed to actually do that, but i suspect you don't need cypress or webdriver to test something like this has the correct output

  <Countdown time={-1} />
  <Countdown time={0} />
  <Countdown time={1} />

or likewise you can probably use sinon or jest's fake timers to test the hook (however it is hooks are tested without triggering that error about not calling hooks outside of a function component, i guess you need to mock React.useState?).

but like, whatever works for your team! i think it's fair to argue for either direction, but neither is zero-cost unless you have buy-in for one direction vs another from your coworkers, which honestly is all that matters especially if you have to eat lunch with them occasionally.

throwaway2902y ago

I like the library, but isn't createOverride / useValue basically modifying components to allow testing?

kolodny2y ago

Creating an override is basically just providing a placeholder for a value to be injected via React Context. I view this as a form of dependency injection. Contrast this with how this would be done in vanilla Playwright with reading it from a query param or exposing a global to call page.evaluate on which is more along the lines of forcing test code into a component.

Note that if you needed a specific reference in an override there isn't a good way to get that via Playwright, consider this silly example:

    it('can test spies', async () => {
      const spy = browserMock.fn();
      const { page } = await render(
        app =>
          <LogoutButtonAction.Override with={() => spy}>
            {app}
          <LogoutButtonAction.Override>
      );

      await page.getByText('Log out').click();
      expect(await spy).toHaveBeenCalled();
    });

1 more reply

thom2y ago

Does it not feel a little old-fashioned to dynamically rebind stuff to force some code to be testable, rather than just write it to be testable in the first place? If I saw someone doing this in any other test suite I’d suggest making the dependencies explicit.

j / k navigate · click thread line to collapse

0 comments

kolodny2y ago

The point is that you shouldn't need to rewrite your countdown component to allow testing. Can you provide a snippet of that change and what the test would look like?

The current solutions are good enough to do smoke tests, but nitty-gritty e2e tests don't scale well.

In the example above it's simple

    export const CountdownTime = createOverride(60);
    export const Countdown = () => {
      const initialTime = CountdownTime.useValue();
      const [time, setTime] = React.useState(initialTime);
      React.useEffect(() => {
        const interval = setInterval(() => {
          setTime(t => t - 1);
          if (t === 1) clearInterval(interval);
        }, 1000);

        return () => clearInterval(interval)
      }, []);

      return time > 0 ? <>Time left {time}</> : <>Done</>
    };

    it('tests faster', async () => {
      const { page } = await render(
        app => <CountdownTime.Override with={() => 5}>{app}</CountdownTime.Override>
      );
      await expect(page.getByText('Done')).not.toBeVisible();
      page.waitForTimeout(5000);
      await expect(page.getByText('Done')).toBeVisible();
    });

If I needed something like this, I'd probably also make setInterval an override as well, so I don't need to wait at all, but you get the idea.

nsfmc2y ago

  const useCountdownValue = initialTime => {
    const [time, setTime] = React.useState(initialTime);
    React.useEffect(() => {
      const interval = setInterval(() => {
        setTime(t => t - 1);
        if (t === 1) clearInterval(interval);
      }, 1000);
  
      return () => clearInterval(interval)
    }, []);
    return time;
  }

  const Countdown = ({time}: {time: number}) => {
    return time > 0 ? <>Time left {time}</> : <>Done</>
  }
  
  const ActualCountdownInContextSomewhere = () => {
    const remainingSeconds = useCountdownValue(60)
    return <>
      <Countdown time={remainingSeconds} />
    </>
  }

i'll say, i have never written a test for a hook or looked into what's needed to actually do that, but i suspect you don't need cypress or webdriver to test something like this has the correct output

  <Countdown time={-1} />
  <Countdown time={0} />
  <Countdown time={1} />

throwaway2902y ago

I like the library, but isn't createOverride / useValue basically modifying components to allow testing?

kolodny2y ago

Note that if you needed a specific reference in an override there isn't a good way to get that via Playwright, consider this silly example:

    it('can test spies', async () => {
      const spy = browserMock.fn();
      const { page } = await render(
        app =>
          <LogoutButtonAction.Override with={() => spy}>
            {app}
          <LogoutButtonAction.Override>
      );

      await page.getByText('Log out').click();
      expect(await spy).toHaveBeenCalled();
    });

1 more reply

thom2y ago

j / k navigate · click thread line to collapse