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

Support for composite asserts #75

Open
mbergal opened this issue Sep 17, 2020 · 5 comments
Open

Support for composite asserts #75

mbergal opened this issue Sep 17, 2020 · 5 comments

Comments

@mbergal
Copy link

mbergal commented Sep 17, 2020

This is pseudo code illustrating the use case (api test)

describe("Authentication service", () => 
  testPromise("Should save user", ()=> {
    let requestResult = client->ServiceClient.saveUser(...)
    let savedUser = // retrieve saved user
         
    expect(requestResult) |> toBe(...)  
    && expect(savedUser) |> toBe(savedUser) 
    });
});
@glennsl
Copy link
Owner

glennsl commented Oct 18, 2020

Earlier discussion in #8. I'm not entirely opposed to it, but I have yet to see a good design and use cases that really need something like this.

@vmarcosp
Copy link

vmarcosp commented Apr 25, 2021

@glennsl There are a lot of cases that need something like this. Take a look at this simple example using jest + react-testing-library:

test('checkboxes (and radios) must use click', () => {
  const handleChange = jest.fn()
  const {container} = render(<input type="checkbox" onChange={handleChange} />)
  const checkbox = container.firstChild
  // for checkboxes, the event that's fired is the click event,
  // and that causes the change event handler to be called.
  // learn more: https://github.com/testing-library/react-testing-library/issues/156
  userEvent.click(checkbox)
  expect(handleChange).toHaveBeenCalledTimes(1)
  expect(checkbox.checked).toBe(true)
})

Example from Kent Dodds's workshop

It's really simple, but you can see that is often to have more than one expect inside of a test / it. Since jest supports it, makes sense to me that these bindings should also support too.

@glennsl
Copy link
Owner

glennsl commented May 2, 2021

I understand your frustration @vmarcosp in being artificially restricted from doing these kinds of things, but this is also exactly the kind of thing it is meant to discourage.

This is IMO not a well-written test, as it tests two completely different aspects in a single test. This leads to a terribly vague test name, and assertions that mask other assertions. In this case, if the handleChange assertion fails, you don't know if checkbox,checked has been checked or not until you've gotten past the other assertion, and this might mask the actual underlying root cause.

In this simple example it might seem like an exaggerated concern, but what is the general principle that decides whether an assertion should be separate or included in another test? To take it to an extreme, why not put everything in a single test? It seems to me that the ideal is one assertion per test, and that the only reason you put multiple assertions in a test is code reuse. We already have good and appropriate code reuse mechanisms. They're called functions. jest itself even has a code reuse mechanism, beforeEach /beforeAll`` and afterEach/`afterAlll`.

So I'd encourage exploring these and making arguments against them as code reuse mechanisms, if that is the sole purpose for wanting this. If it's not, then I'd need more of an explanation to understand the use case and problem.

@vmarcosp
Copy link

vmarcosp commented May 3, 2021

Hey, thanks for answering!

I understand your points and I disagree that is not a well-written test. But, my point is not if is a good or a bad test. My point is: Since jest supports multiple assertions in a test, its rescript bindings should support too. I know that you have good intentions with this restriction and I think it's a good idea, especially when dealing with bindings for a javascript library with a poor API.

@daniel-nagy
Copy link

daniel-nagy commented Oct 1, 2022

I think most people just want the bindings without any opinions on how they should be authoring tests with Jest. Especially people migrating existing apps. This is one more hurdle in their way.

To say having a single test with multiple assertions is bad is subjective. Given no technical reason for the limitation I think teams should be able to decide if that is good or bad. I think this is easily defeated anyway by wrapping the assertion in a function that returns a unit.

For example, is this test bad?

expect(foo).toHaveBeenCalledTimes(1);
expect(foo).toHaveBeenCalledWith(bar);

In my opinion this test is acceptable.

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

4 participants