refactor(labware-creator): break apart footprint section

[shlokamin]

Overview

Follow up PR to #7735

Break apart footprint section, move stuff around and add missing test coverage.

Also adds @testing-library/jest-dom as a dependency and global test import as part of the jest config.

Risk assessment

Low

[shlokamin]

I tried to use jest-when by doing this:

when(radioFieldMock)
      .expectCalledWith({
        name: 'homogeneousWells',
        options: [
          { name: 'Yes', value: 'true' },
          { name: 'No', value: 'false' },
        ],
      })

but for some reason the mock never gets invoked and I couldn't figure out why. too much to do will look into this another time

[shlokamin]

in case anyone wants to know why this isn't working, its cuz React calls functional components with two parameters... one is props, and one is some sort of refOrContext. https://stackoverflow.com/a/57417184

thanks @mcous for noticing this and pointing it out, I learned something new today!

[shlokamin]

this was leftover from debugging a previous PR

[shlokamin]

adding this so its easier to write tests with test library + jest. maybe importing this in all tests should go as part of a global test setup or something @mcous @b-cooper @Opentrons/js

[shlokamin]

see comment below about global setup for this import

[Kadee80]

LGTM but i defer to others on the package.json questions

[mcous]

Out of curiosity, why are you using expectCalledWith over calledWith? These days, I tend to agree with the critiques outlined here about this style of setting expectations before exercising the subject under test, but I'm curious to hear your thoughts

(TL;DR: this violates arrange-act-assert and may place unreasonable constraints on the subject under test)

[shlokamin]

Sorry for the long response, mainly thinking out loud...

this sentence sort of blew my mind:

if the subject can magically figure out a way to solve the problem without needing to invoke a configured stubbing, it shouldn't be punished for doing so by triggering a failure

I need to mentally work through this a bit... let's say I did this instead:

when(getIsHiddenMock)
      .calledWith('labwareType', {} as any)
      .mockReturnValue(true)

If getIsHiddenMock gets called with another unexpected parameter, it would return undefined, which would cause the test to fail because now this statement is no longer true: numFieldsHidden === fieldList.length.

So we're good there, our test would fail as it should.

If, for some reason, the test still passed even though getIsHiddenMock gets called with an unexpected parameter, what would that mean? (genuine question). In JS world it gets weird, because it means that the mock returning undefined is sufficient for the test to pass. But what does that mean??

It's easier to think of the opposite example, instead of getIsHidden, let's say it were getIsVisible. If the module where getIsVisible lives got mocked via jest.mock, getIsVisible would return undefined if called with unexpected parameters. That means that the folllwing mock would be sort of meaningless:

when(getIsVisibleMock)
      .calledWith('labwareType', {} as any)
      .mockReturnValue(false)

because in getFormAlerts the values from getIsVisible get casted to booleans, so undefined would get casted to false anyways.

So I guess in a sense, my hope was that using expectCalledWith would be an additional "fail safe" around the test to make sure weird unexpected things don't happen. A way around this so we don't violate arrange - act - assert (as the post mentions) would be to use a spy instead, and assert that the spy was called with certain values.

anyways, the critique you linked makes the case that using something like expectCalledWith

is an indication that the assertion is unnecessary

I don't follow that though, because the assertion is still proving value. We are making sure that when all of the fields are hidden, getFormAlerts returns null. This is accomplished whether or not we use expectCalledWith, or used a spy instead.

IMO expectCalledWith is really just a way to organize the test in a way that groups logical pieces together. Let's compare using a spy vs expectCalledWith:

// Spy example
it('should return null when all fields are hidden', () => {
    const getIsHiddenMockSpy = jest
        .spyOn(formSelectors, 'getIsHidden')
        .mockReturnValue(true)

    const props: FormAlertProps = {
      values: {} as any,
      fieldList: ['labwareType'],
      touched: {},
      errors: {},
    }
    expect(getFormAlerts(props)).toBe(null)
    expect(getIsHiddenMockSpy).toHaveBeenCalledWith('labwareType', {})
  })

// expectCalledWith (as is in this PR)
it('should return null when all fields are hidden', () => {
    when(getIsHiddenMock)
      .expectCalledWith('labwareType', {} as any)
      .mockReturnValue(true)

    const props: FormAlertProps = {
      values: {} as any,
      fieldList: ['labwareType'],
      touched: {},
      errors: {},
    }
    expect(getFormAlerts(props)).toBe(null)
  })

In the second snippet, rather than asserting that the mock was called with certain parameters after we arrange and act, we group it together with the mock's behavior itself, thereby keeping the mock's logic all in one place. I don't necessarily see this as a bad thing... but I can see how mixing behavior and assertions in one place might be confusing.

The question as to whether making sure getIsHiddenMock gets called correctly is even necessary (which i realize now might have been what the author actually meant) is a different question. It's true that we're sort of intruding on the implementation details of whatever we're testing. It's also true that we're already replacing implementation details of the module with another implementation that we are making up (i.e. the mock), so I think it's fair to make sure that the "fake" implementation is being invoked in the way we expect. If it's not, this might be an indication that there is something unexpected happening behind the scenes (which I suppose could be argued is out of scope of what the test is actually supposed to be checking for).

[mcous]

So I think we're mostly agreeing! My TL;DR is that I prefer calledWith over expectCalledWith, because I find the assertion that expectCalledWith tacks on to the argument configuration to be unhelpful.

I agree that we care about how the subject interacts with its dependency. We're testing contracts, we need to use the test to clearly communicate that interaction spec.

In certain pedantic nomenclature, expecteCalledWith would be a "mock" and calledWith would be a "stub":

// expectCalledWith - mock
when(getIsHiddenMock)
    .expectCalledWith('labwareType', {} as any)
    .mockReturnValue(true)

// calledWith - stub
when(getIsHiddenMock)
    .calledWith('labwareType', {} as any)
    .mockReturnValue(true)

The difference between the two here is that in option 1, the test will assert if the mock is called in any other way, whereas option 2 will simply no-op. This means option 2, the stub, does not violate arrange-act-assert, because the stub configuration is simply arrangement; it has no bearing on assertions except that the subject under test may produce incorrect data and fail a later assertion if it interacts with the stub incorrectly.

So, given these two options, when my original link says about option 1:

It conflates assertion (via expect) with stubbing (via andStubReturn), which explicitly verifies the implementation of the subject and not its ultimate value (i.e. if the subject can magically figure out a way to solve the problem without needing to invoke a configured stubbing, it shouldn't be punished for doing so by triggering a failure). This is an indication that the assertion is unnecessary

(Emphasis mine)

This is not saying the the argument configuration is unnecessary. With option 1, the mock with an assertion, you are configuring a double that says:

• If I'm called in this way, I will return this value
• If I'm called in a different way, I will assert and fail the test

If the code under test is properly designed, it's going to require the correct return value from its dependency in order to produce its own correct output. If the subject misuses the dependency or its data, the test should fail due to incorrect output from the subject. Therefore, if the test would fail due to bad output if the dependency is misused thanks to the argument configuration, the assertion that the mock throws into the mix isn't doing anything from a pass/fail standpoint.

I think there's an argument to be made that it can produce easier to follow failure message by violating AAA, but this argument doesn't tend to move me for a couple reasons:

• Tests that mock this way aren't really here for refactor protection; they're here for design pressure, so I see less value from assertion messages compared to tests on a pure function that are really focused on opaque inputs and outputs
• Call verification can punish the subject if it calls the dependency in ways additional to the correct one, even when they have nothing to do with the functionality being tested. This can lead to test pain through needless failures

Also, I very much agree that spy-then-verify is not good here:

const getIsHiddenMockSpy = jest
  .spyOn(formSelectors, 'getIsHidden')    // bad because it's a partial mock of formSelectors
  .mockReturnValue(true)  // bad because the return value is unconditional
// ...
expect(getIsHiddenMockSpy).toHaveBeenCalledWith('labwareType', {}) // bad because hard to follow while reading

[mcous]

Oops, forgot to answer though on this particular question:

If, for some reason, the test still passed even though getIsHiddenMock gets called with an unexpected parameter, what would that mean? (genuine question). In JS world it gets weird, because it means that the mock returning undefined is sufficient for the test to pass. But what does that mean??

Maybe it just means you're not done writing tests! I sorta view a test passing even if a configured stub isn't called the same as any other TDD cycle. I can write:

const isEven = () => false

it('should handle odd numbers', () => {
  expect(isEven(1)).toBe(false)
})

My tests all pass, but that doesn't mean the code works.

And this isn't just a JS thing! IIRC Mockito (effectively the same API as jest-when) is Java, and its stubs don't no-op by default. Instead they return whatever is "falsiest" for their type - so a stub of a function typed () => boolean would return false by default

[shlokamin]

I think you're right about this:

Maybe it just means you're not done writing tests!

and this

If the code under test is properly designed, it's going to require the correct return value from its dependency in order to produce its own correct output. If the subject misuses the dependency or its data, the test should fail due to incorrect output from the subject. Therefore, if the test would fail due to bad output if the dependency is misused thanks to the argument configuration, the assertion that the mock throws into the mix isn't doing anything from a pass/fail standpoint.

The part that makes me double take is this though:

If the code under test is properly designed

I think because we are imperfect humans writing imperfect tests for imperfect software, our tests can help us determine whether or not our code is properly designed. If we see that a dependency is being misused by the subject via mocks, we can use that info in order to better design our software.

I still need to watch the video you sent me, my thoughts on this topic are quite loose and not very well grounded so I really appreciate this discussion :)

[shlokamin]

Also, I'm gonna merge this PR for now so @Kadee80 can use it to break out more sections, but i'd love to keep the convo going and potentially document new opinions and have a follow up PR.

[mcous]

If the code under test is properly designed

I think because we are imperfect humans writing imperfect tests for imperfect software, our tests can help us determine whether or not our code is properly designed

This is a really good point, and I think I probably should've written "If the code under test is properly designed (by going through TDD)" or something like that. For me, I think that expectCalledWith (specify mock behavior and assert) produces false design pressure compared to calledWith (specify mock behavior without assert). Perhaps "over-constrained" is a better way to put it than "false".

To go back to our (definitely over-simplistic for this discussion) example where we have a subject under test that is supposed to do something with a true/false return from a dependency. We write our first test to specify the false behavior:

if('should return "xyz" when flag is false', () => {
  // stub: simply specify behavior when called correctly
  when(getFlag).calledWith('flag-name').mockReturnValue(false)
  // or mock: specify behavior when called correctly _and_ assert if called incorrectly
  when(getFlag).expectCalledWith('flag-name').mockReturnValue(false)

  const result = subject()
  expect(result).toBe('xyz')
})

With the stub, if I'm being very TDD, I might do:

const subject = () => 'xyz'

And this would pass! But with the mock, this wouldn't pass (I'm actually pretty sure it won't assert just because the mock isn't called, but let's assume not calling the mock will assert as an easier-to-type stand-in for calling the mock incorrectly in this example). So if I was mocking, I need to make sure my subject doesn't hit that assert. Accordingly, I'll feel undue pressure to call the dependency and maybe even start thinking about other return values:

const subject = () => getFlag('flag-name') ? '123' : 'xyz'

So the crucial part for me about "over-constrained design pressure" is that now I when go to write my second test case:

it('should return "123" when flag is true', () => {
  // stub: simply specify behavior when called correctly
  when(getFlag).calledWith('flag-name').mockReturnValue(true)
  // or mock: specify behavior when called correctly _and_ assert if called incorrectly
  when(getFlag).expectCalledWith('flag-name').mockReturnValue(true)

  const result = subject()
  expect(result).toBe('123')
})

My second test case fails in my stubbing example, because I did exactly enough to get the first test without considering any other data or behaviors. My code written so far does not interact with the stub (or interacts incorrectly in a more sophisticated example), so it obviously returns the wrong value. A failing test is design pressure.

However, in my mock example, my second test case passes right away because I was pressured by expectCalledWith into interacting with the stub during the first test, even though I wasn't yet testing other outputs from my subject. While I might feel good that I've got all greens, my second test case exerted no useful design pressure. Since, in my opinion, mock-based tests are most useful for their influence on design, writing a test case that doesn't exert design pressure means I have trouble articulating the value of that test.

You might say that my mock example above is a bit of a strawman (and it is!), but I think it would be more realistic to write my above example than something that both passes the mock assert on arguments and allows you to write a failing test for the true test case:

const subject = () => {
  getFlag('flag-name')  // eslint will complain about this, which is more design pressure
  return 'xyz'
}

Heavy caveats here given the overly simplistic example and the fact that a tight red-green loop isn't always realistic, but I think this is the most useful demonstration of where I'm coming from. If a dependency provides data, I think design pressure should come from assertions on data, not from assertions on interactions

[shlokamin]

So if I was mocking, I need to make sure my subject doesn't hit that assert. Accordingly, I'll feel undue pressure to call the dependency and maybe even start thinking about other return values

My second test case fails in my stubbing example, because I did exactly enough to get the first test without considering any other data or behaviors. My code written so far does not interact with the stub (or interacts incorrectly in a more sophisticated example), so it obviously returns the wrong value. A failing test is design pressure.

These are very strong arguments, and tie back nicely with this:

Since, in my opinion, mock-based tests are most useful for their influence on design, writing a test case that doesn't exert design pressure means I have trouble articulating the value of that test.

I think we're getting into something deeper now, which might boil down to why do we write tests?.

I think (and correct me if I'm wrong) you are making the argument that a core reason we write tests is to (hopefully positively) influence the design of the software itself. Here's where TDD comes in, because that's precisely what it's supposed to do. It forces you to design your software from the perspective of the consumer, in a way that is by definition testable.

I agree that this is a core reason why I personally write tests (though ironically enough in this refactor I did not practice TDD, shame on me 🙈 ). Having grown up (read: brainwashed) at ThoughtWorks, this reason alone moves me enough to move away from reaching for something like expectCalledWith.

I do wonder how others feel though, because to me the crux of the argument against using something like expectCalledWith comes down to the reason why we even write tests in the first place. And at this moment in time, I don't think we all agree on what those reasons are. IMO it would be really neat to have a unified opinion as an org. I know that's a slippery slope and some people might see that as big brother like, but I do think some sort of shared philosophy on this subject is important because it really does have strong implications on system design.

[mcous]

I think we're getting into something deeper now, which might boil down to why do we write tests?.

I think (and correct me if I'm wrong) you are making the argument that a core reason we write tests is to (hopefully positively) influence the design of the software itself. Here's where TDD comes in, because that's precisely what it's supposed to do. It forces you to design your software from the perspective of the consumer, in a way that is by definition testable.

Yes, exactly this. The thing that's been a recent shift for me is accepting that different tests provide different value. To continue being overly simplistic:

• Tests with mocks (to use the term generally) provide design pressure, without really providing regression safety
• Tests that treat the subject as an opaque input/output box provide regression safety, without really providing design pressure

Obviously, there are shades of grey when you actually get into things, but I like taking this approach, because you can use mocks to design your system, split up into however pure data providers (or side-effectors) that you need, and then drop into black-box testing (for both the units and integration testing) to get your regression safety once you've got a system design

[IanLondon]

hahaha

[shlokamin]

ugh caught me