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

test: more concise test helpers and unit tests #33

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

RJWadley
Copy link
Contributor

@RJWadley RJWadley commented Feb 5, 2025

as I was playing around with the createUnitTest helper I noticed when testing basic syntaxes I was repeating myself a lot.

I've got duplicate html in test and expect, as well as duplicate css as both an object and string, and for nesting tests I've been testing 3 separate behaviors. and it multiplies, so you for simple unit tests you end up with thousand line unreadable test files with lots of repetition.

I've made createSyntaxTest and createNestingTest to help make tests more concise and readable on this front.

createSyntaxTest

createSyntaxTest is a simple wrapper over createUnitTest that generates matching html and CSS for your test case. This is particularly useful when your testing a specific syntax, for example a media query:

createSyntaxTest({
  name: '@media at-rule',
  css: {
    '@media (min-width: 1000px)': {
      backgroundColor: 'blue',
    },
  },
  children: <div>@media</div>,
})

children can also accept a function, if you need to nest a styled element within itself.

createSyntaxTest({
  name: '@scope at-rule',
  fails: true,
  css: {
    '@scope (.start) to (.end)': {
      backgroundColor: 'blue',
    },
  },
  children: (This) => (
    <>
      <This />
      <div className="start">
        <This />
        <div className="end">
          <This>@scope</This>
        </div>
      </div>
    </>
  ),
})

createNestedTest

createNestedTest is a targeted utility for testing the behavior of & combined with a specific selector

createNestingTest({
  name: 'id selectors',
  children: <div id="a" />,
  css: {
    backgroundColor: 'red',

    '#a': {
      backgroundColor: 'blue',
    },
  },
})

the above example tests the behavior of #a, &#a and & #a as one suite.

Copy link

vercel bot commented Feb 5, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
restyle ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 8, 2025 3:35am

- firefox has a tendency to hang, which makes testing difficult
- safari has some support differences which make initial test creation a bit less convenient

I'd like to re-add both these once vitest's browser mode is a bit more stable & most of restyle's tests are passing
@souporserious
Copy link
Owner

It took me a bit of reading through the existing tests to fully understand how these new utilities work. I’m curious about your thoughts on using a more declarative snapshot-based API that would more closely mimic how the library is used in practice while still reducing duplication? Thinking something along these lines if possible:

import { test, expect } from 'vitest'
import { render } from 'vitest-browser-react'

test("@scope at-rule", () => {
  const Div = styled("div", {
    "@scope (.start) to (.end)": {
      backgroundColor: "blue",
    },
  });

  const screen = render(
    <Div>
      <Div />
      <div className="start">
        <Div />
        <div className="end">
          <Div>@scope</Div>
        </div>
      </div>
    </Div>
  );
  
  expect(screen).toMatchSnapshot(`{/* expected CSS styles here somehow */}`);
});

I’m not entirely sure if Vitest’s browser mode supports snapshots yet, but would this remove the need for things like expect and css options in createUnitTest? Alternatively, we could instead consider testing createRules / createStyles for these scenarios directly to assert that the generated class names and styles match what's expected. There's also renderToStaticMarkup from react-dom/server that could be a simple way to assert that the generated class names match what's expected.

I definitely see why these abstractions exist, and they seem helpful. That said, I’d like to explore a more intuitive approach for now, and in general, I’d lean toward a bit more verbosity before introducing too many abstractions.

@souporserious
Copy link
Owner

Thinking about this more, using the css prop could further help in these situations if we can get snapshots working:

Before

createUnitTest({
  name: 'deeply nesting id selectors',
  test: styled('div', {
    color: 'red',

    '#a': {
      color: 'green',

      '#b': {
        color: 'blue',
      },
    },
  })({
    children: (
      <>
        <div id="a">
          <div id="b" />
        </div>
        <div id="b" />
      </>
    ),
  }),
  expect: (
    <div className="a">
      <div id="a">
        <div id="b" />
      </div>
      <div id="b" />
    </div>
  ),
  css: css`
    .a {
      color: red;
      #a {
        color: green;
        #b {
          color: blue;
        }
      }
    }
  `,
})

After

test("deeply nesting id selectors", () => {
  const screen = render(
    <div
      css={{
        color: "red",

        "#a": {
          color: "green",

          "#b": {
            color: "blue",
          },
        },
      }}
    >
      <div id="a">
        <div id="b" />
      </div>
      <div id="b" />
    </div>,
  );

  expect(screen).toMatchInlineSnapshot(``)
});

Creating a restyle-specific render helper could allow for options e.g. render(...).shouldFail()

@RJWadley
Copy link
Contributor Author

RJWadley commented Feb 10, 2025

Yeah, I'm actually not a huge fan of the multiple utilities here. although it does make writing tests very easy.

As far as snapshot testing in general goes, my biggest concerns are:

  • It's not really a hard requirement, but I would like to validate every CSS property. That would make snapshots a bit large.
  • I don't know what the correct values are for a specific snapshot, and I don't really care. I just want to match the browser. I don't think snapshots are that great of a medium for this kind of test. For example, in that test for @scope I don't know or care about which div has which color, and I don't really need to- just match whatever the browser wanted.

Otherwise, there are really two broad kinds of tests we'll be dealing with as far as E2E/unit testing goes.

testing specific CSS syntaxes and features.

there's generally a lot of these, so I personally would prioritize shorter, easier to write tests with utilities handling the heavy lifting. These are also more likely to be written by maintainers who are familiar with the project.
I think the createNestedTest and createSyntaxTest are pretty ideal here API wise. We could maybe do something declarative.

The names (and the docs) kinda suck tho, open to ideas.

testing specific CSS issues

behaviors, edge cases, and things like that. there are generally fewer of these, and aren't really exhaustive, mostly just to prevent regressions on things. These will also be written gradually over time, so I'd prefer a more stable, versatile format even if more verbose. These are also more likely to be written by first-time contributers, since they'll often accompany github issues or pull requests.

I do actually love the idea of a declarative api where we can test more intricate behaviors, we'll probably need this at some point anyway. maybe something like this?

test("@scope at-rule", () => {
  const Div = styled("div", {
    "@scope (.start) to (.end)": {
      backgroundColor: "blue",
    },
  });

  const screen = render(
    <Div>
      <Div />
      <div className="start">
        <Div />
        <div className="end">
          <Div>@scope</Div>
        </div>
      </div>
    </Div>
  );
  
  // looks for the specified tag structure with the specified styles,
  // doesn't touch the test's DOM, which would be an improvement over the current utils
  expectMatchingStyles(
  html`
    <div className='a'>
      Hello World
    </div>
  `,
  css`
    .a { color: red }
  `
  )
});

I'd be pretty happy if something like that replaced createUnitTest, meanwhile createSyntaxTest and createNestedTest lived on with better names and used the new utils under the hood.

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

Successfully merging this pull request may close these issues.

2 participants