Rename fixtures associated with org_dogs_test integration test file

[Eduardo06sp]

Summary of changes:

• Renamed fixtures referenced in test/integration/org_dogs_test.rb
  • I had to edit associated fixtures as well, that were not explicitly used in this file
• Refactored fixture associated with test/integration/user_account_test.rb:64
  • Deleted a fixture used solely for this test, I was able to just add an ID of 1 to another existing fixture to pass this (see commit: f948dcc)
• Removed unused fixture files
• Removed unnecessary comments from fixture files

Explanation for changes:
Overall, I opted to go for the most simple naming convention that was apparent. I preferred slightly longer names such as verified_staff_one for the sake of clarity.

I gave the most generic fixtures basic numbers as names, since they do not represent anything special (that I noticed) throughout the test suite. E.g. instead of organization_one, I renamed it to one, which very nicely gets referenced as organizations(:one).

I constantly double-checked my work & re-ran tests as I committed to ensure tests would pass. Please let me know if there are still any changes you would like to see.

Related issue: #149

[kasugaijin]

Thank you @Eduardo06sp that cleans things up a bit and makes for easier comprehension.
The more I learn about fixtures and FactoryBot I feel sometimes it would be better to use one over the other. I get a little conflicted when we name a fixture based on the state of one of its associations. For example, naming a dog 'pending_adoption` or the likes, it makes sense based on its initial state set by the fixtures. But, I feel like it can become a burden or source of confusion in a larger integration test where, say, the dog fixture with that name becomes adopted, or the application is withdrawn, and then its name no longer reflects its state in that point of the test. However, right now I think the names you have proposed make sense and are better than what I had used originally.

In restrospect, I think this test suite could use a blend of fixtures and FactoryBot. Fixtures could be used to create instances of the more foundational models that experience less change e.g., organization or an adopter user or staff user, and then FactoryBot could be used for models that do experience change e.g., Dog, Adoption Application etc. This would allow you to create these objects as needed for a test, naming it what you wanted for that particular use case, and avoid the confusion I mention above.

I think fixtures have a trade off. They can save time, but they also require more overhead and naming is subject to being confusing the more those instances are being manipulated in the test suite.

This is just an evolution of my thinking since building this app at first, and I am curious on your thoughts. I am not at all saying things need to change right now. But I think it's worth discussing a bit before jumping into more refactoring.

[Eduardo06sp]

No problem @kasugaijin! I will admit that as I was implementing these changes (as recommended by Jason Swett), I had some thoughts as to what direction I would have taken.

I do see your concern about giving these "specific" names for a state they're in. Again, I did my best to follow the advice from the video. That is, to name your fixtures based on what was particularly "special" about a given fixture.

There were various instances in which the state of a fixture really didn't matter or make sense for a given test. E.g. testing that staff can edit a dog and use dogs(:pending_adoption_one), which does not seem to make sense.

Here is one approach to fixtures.
Have a very minimal baseline set of fixtures defined with the minimum test data required to be valid, then fully "fleshed" out ones with everything filled out like in a "real-world" case. These fixtures should be used for most tests, and as soon as you start needing to change a property or two, or maybe more, consider how often you need to make that change.

If it's just one or two tests, it can be best to just straight up update the fixture inside the test itself. If you start seeing a pattern of "wow, I keep needing certain things in this part of my tests" then maybe an extra fixture (or factory) should be created. Also, do not be afraid to simply create new objects as well if it makes more sense for a given test.

As you can see, this kind of changes the fundamental approach to creating test data. I'm assuming that's an approach you would want to take, regardless of the approach used (fixtures only or mixing in FactoryBot).

[Eduardo06sp]

I personally don't think that the tests are "littered" with too many fixtures, nor do I think that they look too unwieldy. For now, the suite looks fine as it is and as with anything, has room for improvement.

If you are confident that adding factorybot will be the best decision, it would be useful to have a new Github Issue for someone to pick up! It'll definitely require some time & thought as to how exactly it would be approached.

[kasugaijin]

Yes I agree with your suggestions above. For now I think the fixtures are doing their job well enough so I don't think we need any changes in how we're handling the tests. I don't see much benefit right now to making any substantial changes and starting to use FactoryBot instead. It's more of a clean up job like you have done in this PR and decoupling the tests from their currently tightly coupled state e.g. testing for counts of specific page elements in all cases.

[Eduardo06sp]

Thank you for bringing that up by the way, it's certainly something to keep in mind!

I will avoid submitting the next commit for this PR and will await further feedback. Take your time! :}