Eliminate checking factory items in our tests

[cielf]

Summary

Eliminate any dependency on factory items in the test code - step 1 - find them all.

Why?

More robust testing

Details

There shouldn't be any tests that rely on factory items having specific values or that check against factory items.
Note: Look especially at any tests that are dependent on sorting or pagination.

Review the tests with an eye to eliminating any checks against factory items. If there are very few, you may go ahead and submit
PRs to fix them, but let's assume you'll need to compile a list

That list should include, for each affected test:
source file, test description (i.e. context, describe, etc) and approximate line number

If you want to review some, and give us a list including what has been reviewed, so some one can pick things up afterwards, that's fine too.

Here is a list of all the factories:

• Partner factories:
  • Child
  • Family
  • Item Request
  • Profile
  • Served Area
  • User
• Account Request
• Adjustments
• Audits
• Barcode items
• Base Items
• Broadcast announcements
• Counties
• Distributions
• Donation Site
• Donations
• Inventory Items
• Item Categories
• Item Units
• Items
• Kit Allocations
• Kits - WIP [Phase 1 Kit Refactor] Fix #3707 add itemizable item, migrate kit line items (backup db before merge) #4560
• Line items
• Manufacturers
• NDBN members
• Organizations
• Partner groups
• Partner user
• Partners
• Product drive participants
• Product drive
• Purchases
• Questions
• Requests
• Roles
• Storage Locations
• Transfers
• Units
• User roles
• Users
• Vendor

Criteria for completion

• lists of affected tests, as described above

Bonus round

• submit PRs to fix some or all of the tests

[veezus]

Does this mean we'd be looking for tests that are asserting against default factory values? Like, a validation spec that assumes a factory creates a valid object?

In the pagination example, we'd want factoried objects with spec-local values to make sure we control like which items appear in the list, as opposed to relying on values in the factory file?

[elasticspoon]

Pretty much yes, for example:

#4099 (comment)
#4099 (comment)

[dorner]

Yeah. My concerns with using factory values are:

1. It's brittle. If the factory ever changes, all the specs break and have to be changed as well.
2. It's confusing. If the value "FOOSPAZ" is given as a default in the factory, and the spec checks that a particular value is "FOOSPAZ", we have to intuit that that value came from the factory and now have two files contributing to that spec (the factory and the test itself).
3. Relying on factory values puts too much power into the factory, and makes us want to use it more. This is how we get to where we are now, where we auto-create a whole bunch of stuff that half the time we don't need.

Specifying exactly the values we care about is a bit more typing, but makes it much easier to follow and change the tests. Factories (IMO) are basically there to fill in values that are required (due to validations etc.) but we don't actually care about or want to check in the current test.

[cielf]

Hey @dorner - We've had a couple of adjacent but not completely solving PRs around this. Are we at a point where this can be either closed or reworked into something narrower?

[dorner]

The PRs around this were more around making sure we don't create data that we don't need. This is more about not checking values that we didn't create ourselves. They don't really have anything to do with each other.

I think we can handle this similarly to the other set, in that someone just needs to go slowly and methodically through the tests and bunch the fixes together into PRs.

[jimmyli97]

If I'm not mistaken I think #4534 (see this comment) is related to this, there's inconsistent behavior with regards to whether the inventory gets affected by different factory objects

[jimmyli97]

I'm working on this for kits as part of working on #3707

here's a checklist of all factories + link to my draft PR for kits checkbox, @cielf maybe put this in the issue description?

Factories to refactor:

- [ ] Partner factories:
    - [ ] Child
    - [ ] Family
    - [ ] Item Request
    - [ ] Profile
    - [ ] Served Area
    - [ ] User
- [ ] Account Request
- [ ] Adjustments
- [ ] Audits
- [ ] Barcode items
- [ ] Base Items
- [ ] Broadcast announcements
- [ ] Counties
- [ ] Distributions
- [ ] Donation Site
- [ ] Donations
- [ ] Inventory Items
- [ ] Item Categories
- [ ] Item Units
- [ ] Items
- [ ] Kit Allocations
- [ ] Kits - #4560
- [ ] Line items
- [ ] Manufacturers
- [ ] NDBN members
- [ ] Organizations
- [ ] Partner groups
- [ ] Partner user
- [ ] Partners
- [ ] Product drive participants
- [ ] Product drive
- [ ] Purchases
- [ ] Questions
- [ ] Requests
- [ ] Roles
- [ ] Storage Locations
- [ ] Transfers
- [ ] Units
- [ ] User roles
- [ ] Users
- [ ] Vendor

[dorner]

@jimmyli97 I hope you aren't putting all the factory rework and the kit changes together? They need to be separate.

Also, I'm not sure the factories themselves need to change, so much as calling code needs to provide explicit values to them in cases where they are checking those values.

[jimmyli97]

@dorner ok I will split them up into separate PRs when I get a chance.

What I did for the kit factory was the following:

1. change factory default values to like "Test Name - Don't Match" and numeric values to a number that doesn't show up when searching e.g. "1313", see what rspecs break, and fix those
2. additionally search the specs folder for references to the factory attributes e.g. kit.name and replace with hard coded values

I feel like that would be faster than methodically reading every spec, and also you can split up work by factory rather than by spec, but I guess it's up to whoever ends up tackling this issue as to how they want to do it.

[dorner]

ah ok - so it's less about fixing the factories and more about using them to help detect tests which need to change. 👍