search

rubyforgood / human-essentials

Human Essentials is an inventory management system for diaper, incontinence, and period-supply banks. It supports them in distributing to partners, tracking inventory, and reporting stats and analytics.
https://humanessentials.app
MIT License
464 stars 493 forks source link

Eliminate checking factory items in our tests #4217

Open cielf opened 7 months ago

cielf commented 7 months ago

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:

Criteria for completion

veezus commented 7 months ago

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 commented 7 months ago

Pretty much yes, for example:

https://github.com/rubyforgood/human-essentials/pull/4099#discussion_r1490018847 https://github.com/rubyforgood/human-essentials/pull/4099#discussion_r1492929221

dorner commented 7 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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