Skip to content

Take a pass on improving test suite by unskipping all tests #3008

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.

Sign up for GitHub

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

Jump to bottom
Closed
albertchae opened this issue Sep 10, 2022 · 3 comments
Closed

Take a pass on improving test suite by unskipping all tests #3008

albertchae opened this issue Sep 10, 2022 · 3 comments
Assignees
@albertchae

Comments

@albertchae
Copy link
Contributor

We want to improve the bank test suite (especially removing fixtures for factories) but we know it's a pretty big problem to take on. To scope it down a bit, we will focus only on getting all the skipped tests unskipped.

The possible outcomes for each unskipped test:

  • Test relied on fixture with bad data and is replaced with a factory implementation that passes
  • Test is for invalid behavior, in cases where the test diverged from the implementation code because the test suite wasn't being enforced
    • Fix test to match new behavior
    • Delete test if it turns out to be irrelevant
  • If replacing with a factory isn't possible or too cumbersome at the moment, getting the test to pass with a fixture is an acceptable last resort.
@albertchae albertchae self-assigned this Sep 10, 2022
@albertchae
Copy link
Contributor Author

As of commit 9967209

Finished in 12.45 seconds (files took 4.05 seconds to load)
258 examples, 0 failures, 76 pending

There are 76 tests skipped

This was referenced Sep 12, 2022
Merged
Merged
Merged
Merged
@albertchae
Copy link
Contributor Author

OK after making some headway on this task, I'm seeing a common theme. Lots of failing tests or test complexity are coming from places where bank has to talk to an an external service

While putting in conditionals everywhere to call external services or not based on Rails env being in test/dev/production is one way to approach this, it's probably not the most maintainable long term. This excellent post goes over the 2x2 quadrant that an engineer usually has to decide on before testing with external services https://thoughtbot.com/blog/testing-third-party-interactions. Although there are valid reasons for every box in the quadrant which the blog discusses, I'm personally a fan of testing at the adapter level and generally using a real request to a fake adapter. HTTP level tests tend to be brittle when managing the canned response over time because it's hard to know when they need to be updated and I prefer to run these sorts of integration tests on a live environment if possible.

@maxwofford any thoughts on this?

More resources on testing 3rd party APIs:
https://thoughtbot.com/upcase/videos/testing-interaction-with-3rd-party-apis
https://thoughtbot.com/blog/how-to-stub-external-services-in-tests
https://thoughtbot.com/blog/faking-apis-in-development-and-staging
https://thoughtbot.com/blog/simplify-tests-by-extracting-side-effects

albertchae added a commit that referenced this issue Sep 15, 2022
@albertchae
Unskip UserService::Create spec and migrate to factories (#3010)
2063db0 
#3008

* Unskip UserService::Create

Test passes after skipping call to BankApi service

* Get tests to pass with factories instead of fixtures

This already revealed some hidden assumptions from the fixtures, such
as that user1 was an organizer in event1.

Also, let! was required instead of let because the laziness could cause
the changed count for User and OrganizerPosition to be overcounted since
it would include users or organizer_positions from the original factory creation.
This is one of the downsides of heavy use of lets.

* Clean up lets usage

No more let! required to get the test to pass
contexts share less setup (only event creation)

Also added a missing set of tests, for the case where a user already
exists but is not already an organizer on the event
albertchae added a commit that referenced this issue Sep 16, 2022
@albertchae
Unskip UserService::GenerateToken spec and migrate to factories (#3011)
bdaf451 
#3008

* Unskip UserService::GenerateToken test and fix

This test fell out of sync with the implementation. Apparently
partner_id is now a required parameter, and the token returned by the
service is a LoginToken object, not the token string

* Use user factory instead of fixture

also clean up some of the test setup
albertchae added a commit that referenced this issue Sep 16, 2022
@albertchae
Update GSuite spec to use factory (#3012)
84b8f68 
#3008

* Fix GSuite spec to use factory instead of fixture

* Move g_suite creation inside individual tests

This will keep tests that don't need to persist the model before faster,
since we can just `build` instead of `create`.
albertchae added a commit that referenced this issue Sep 16, 2022
@albertchae
Unskip GSuiteService::Create spec and migrate to factories (#3013)
342f5ae 
#3008

* Unskip GSuiteService::Create specs, they pass with no changes

* GSuiteService::Create tests pass with factories instead of fixtures

* Reorganize test setup

Because fixtures load all the data at the beginning, it looks like the
test was written with the existing case vs non existing case first, but
IMO the happy case is when the gsuite doesn't already exist. With
factories this looks cleaner because data isn't created unless it's
absolutely needed for the test
This was referenced Sep 26, 2022
Merged
Merged
Merged
Merged
albertchae added a commit that referenced this issue Sep 28, 2022
@albertchae
Unskip g_suite_account_service/create_spec.rb and replace fixtures wi…
3220e88 
…th factories (#3046)

#3008

Similar to #3043, move the production specific skip lower in the stack so that the rest of the service's API is upheld.

* Unskip test

Move environment check into specific client wrappers
Because email has text and html, must use body.encoded https://stackoverflow.com/questions/29454102/in-rails-why-is-my-mail-body-empty-only-in-my-test
Fix variables (event was missing, @g_suite is actually g_suite)

* Replace fixtures with factories
albertchae added a commit that referenced this issue Sep 29, 2022
@albertchae
Unskip g_suite_service/mark_verified_spec.rb and replace fixtures wit…
b0f1c36 
…h factories (#3045)

#3008

This set of tests actually passed without doing anything, so not sure why it was skipped. I replaced fixtures with factories and improved the context organization. Frequently tests end up being in a 2D grid, such as

|| verified_on_google == true      | verified_on_google == false |
|-| ----------- | ----------- |
| no created_by|changes state, no email      | no change state, no email       |
| has created_by|sends email   | no change state, no email        |

but code ends up being more "1D", so I find it helps to pick one axis as the top level one and arrange all tests that way. In this PR I chose the `verified_on_google` value

* Unskip GSuiteService::MarkVerified spec and everything passes

* Replace fixtures with factories

* Rearrange tests into better contexts.

The top level thing we care about in this service is whether
verified_on_google is true or false, so that should be the top level
context. Then we can have a nested context for each case that checks
behavior when created_by exists
This was referenced Sep 29, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
albertchae added a commit that referenced this issue Oct 4, 2022
@albertchae
Unskip donations_new_spec, fix, replace fixtures with factories (#3085)
64d366c 
#3008

- approved was changed to the default state in #1938
- delete an irrelevant test
albertchae added a commit that referenced this issue Oct 6, 2022
@albertchae
Unskip g_suite_mailer_spec and convert fixtures to factories (#3067)
6f08e19 
#3008

The main issue here is that the URL being checked didn't conform to the actual structure for the event's gsuite overview. With factories we have to make it more dynamic, but because the implementation is using the event_g_suite_overview_url helper, it'd be kind of a tautological test, so I checked for an expected URL built with File.join instead.


* Unskip g_suite_mailer_spec and fix

It looks like we are looking for the gsuite event URL, which is not the
URL that was in the test

* Convert fixtures to factories

This required updating the URL to be more dynamic. We could have used
the event_g_suite_overview_url helper but that's the same method the
implementation uses so it'd be kind of a tautological test, so I checked
for a URL built with File.join instead.
@albertchae albertchae mentioned this issue Oct 6, 2022
Merged
albertchae added a commit that referenced this issue Oct 6, 2022
@albertchae
Unskip raw_pending_partner_donation_transaction_spec and replace fixt…
653f159 
…ures with factories (#3098)

#3008

This spec passed without any changes after unskipping, but I replaced
the fixtures with factories
@albertchae albertchae mentioned this issue Oct 9, 2022
Closed
@albertchae albertchae mentioned this issue Nov 3, 2022
Merged
albertchae added a commit that referenced this issue Nov 3, 2022
@albertchae
Unskip fee_relationship_spec and fix (#3083)
598ed8c 
#3008

First 2 commits are adding dependent factories for `bank_account` and `transaction` (and updating their model specs at the same time), then the last one creates a `fee_relationship` factory and uses it to refactor the test significantly.

* Replace bank_account fixture with factory

* Replace transaction fixture with factories

* Replace fee_relationship fixture with factory

fix tests, several of these just tested the value was nil after setting
it to nil in the test setup...

* add validates amount to transaction
albertchae added a commit that referenced this issue Nov 4, 2022
@albertchae
Unskip various dry validation contract specs (#3080)
4def0d4 
#3008

All of the specs seem related to "contracts" from https://dry-rb.org/gems/dry-validation/1.5/. They seem to be out of sync with implementation due to missing params or param name changes, so I updated them all to match the current implementation.

- Unskip connect_start_contract_spec and add missing required fields
- Unskip donations_start_contract_spec and fix test
- Unskip generate_login_url_contract_spec and fix
- Unskip login_contract_spec and fix
- Unskip organization_contract_spec and fix
@albertchae
Copy link
Contributor Author

All specs are unskipped and passing and specs are now a required step before merging, so closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@albertchae albertchae

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@albertchae @sampoder @transcental