Skip to content
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

Add feature spec for checkout #127

Merged
merged 5 commits into from
Jan 19, 2022
Merged

Add feature spec for checkout #127

merged 5 commits into from
Jan 19, 2022

Conversation

Noah-Silvera
Copy link
Member

@Noah-Silvera Noah-Silvera commented Nov 22, 2021 •
edited
Loading

What is the goal of this PR?

Add a basic acceptance test for the extension so you no longer have to run the painful manual testing step before merging a PR.

Merge Checklist

  • Update the changelog
    - [ ] Run a sandbox app and verify taxes are being calculated

@Noah-Silvera Noah-Silvera mentioned this pull request Nov 22, 2021
Closed
@benjaminwil
Copy link
Member

Something is up with the Capybara configuration for CI:

ActionController::RoutingError:
  No route matches [GET] "/fonts.googleapis.com/css"
  ...

And I'd be curious to learn why this happened! Very interesting.

@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch from 9133ba9 to b164f78 Compare November 22, 2021 23:50
@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch 4 times, most recently from b7d3be0 to 869e851 Compare November 29, 2021 22:36
@Noah-Silvera
Copy link
Member Author

These tests are failing due to a circle CI problem :( We are just gonna wait a couple days to see if circle CI fixes it.
solidusio/solidus#4211

@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch from 869e851 to 0cee167 Compare December 8, 2021 21:15
@Noah-Silvera
Copy link
Member Author

Just a rebase on master to try and get the tests passing.

@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch 2 times, most recently from 75e6259 to f793e8d Compare December 15, 2021 23:00
@benjaminwil benjaminwil force-pushed the add-feature-spec-for-checkout branch from f793e8d to b3edd9f Compare January 13, 2022 23:18
@benjaminwil
Copy link
Member

Alex and I rebased against master and added made a change to the #fill_in_address helper method so that it works with Solidus 2.x and 3.x.

@benjaminwil benjaminwil force-pushed the add-feature-spec-for-checkout branch 2 times, most recently from 332e87a to 45cdfcc Compare January 14, 2022 23:19
@Noah-Silvera
Copy link
Member Author

This is the command to reproduce the error on CI (with rails 6.1 and solidus 2.11)

bundle exec rspec './spec/features/spree/admin/checkout_spec.rb[1:1]' './spec/super_good/solidus_taxjar/api_params_spec.rb[1:6:1]' --seed 1

@Noah-Silvera
Copy link
Member Author

I figured the test error out and I hate it

  1. The cassette returns data for a item with id:1 and uses that to calculate taxes
  2. The api_params_spec.rbtest that causes the error when run alongside creates a line item first. This causes the id counter to move to 2.
  3. The line item that the checkout spec creates now has an ID that doesn't match the line item returned in the taxjar cassette.

See the passing tests and last commit of this draft PR #147

@Noah-Silvera
Copy link
Member Author

cc: @benjaminwil

@benjaminwil
Copy link
Member

@Noah-Silvera Can we use VCR request matchers and tell them to ignore the ID part of the request? I think if all other parts of the request are the same, that's still reasonable.

@Noah-Silvera
Copy link
Member Author

@benjaminwil it's actually matching ok!

The problem is that the body of the request that is returned contains a line item with id:1
This body is processed by the app, and used to calculate taxes. When the order taxes are calculated in the app, it looks for a line item with id: 2, and doesn't find a match in the TaxJar response, so it calculates zero tax for the line item.

One (brittle) fix is to update the response in the cassette to have the id: 2

Another would be to intercept the VCR response, and modify it to match whatever the order line item ID is, but I don't know how you'd go about that.

@Noah-Silvera @nvandoorn
Configure VCR for feature tests
f512f05 
A feature test that calls out to the real TaxJar api will be added
in a future commit, so we should configure VCR to allow us to record
that request.

Co-authored-by: Nick Van Doorn <nick@super.gd>
@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch from 45cdfcc to 025f182 Compare January 19, 2022 21:44
@Noah-Silvera
Copy link
Member Author

I fixed the ID in the spec and cassette as discussed in slack.

@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch from 025f182 to 198d7b0 Compare January 19, 2022 21:47
@Noah-Silvera
Copy link
Member Author

Added a comment to the change.

forkata
forkata approved these changes Jan 19, 2022
View reviewed changes
Copy link
Collaborator

@forkata forkata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Thanks Noah, Nick and Co.

Noah-Silvera and others added 4 commits January 19, 2022 14:05
@Noah-Silvera @nvandoorn
Add basic extension acceptance test
4559b11 
The most basic test of this extension is "does it calculate tax for
orders"

Since the testing application now automatically configures the
extension, we can just test whether a basic checkout will calculate
tax with Taxjar.

Solidus will try and load the google apis font on a CI where no
default fonts are available, so we should tell VCR to ignore this
request.

Co-authored-by: Nick Van Doorn <nick@super.gd>
@Noah-Silvera @nvandoorn
Remove manual testing step from pull request template
cc20293 
Now that an acceptance test exists to ensure basic tax can be
calculated, we can remove this manual testing step.

Co-authored-by: Nick Van Doorn <nick@super.gd>
@benjaminwil @alexblackie @Noah-Silvera
Fix feature tests on Solidus v2.x
35548fa 
Solidus 2.11 introduces a single name field for the customer's billing
and shipping contact information. Solidus < 2.11 used `firstname` and
`lastname` fields.

Now, this test passes on all the versions of Solidus this extension
supports.

Co-Authored-By: Alex Blackie <alex@super.gd>
@Noah-Silvera
Update changelog
d52a41a
@Noah-Silvera Noah-Silvera force-pushed the add-feature-spec-for-checkout branch from 198d7b0 to d52a41a Compare January 19, 2022 22:05
@Noah-Silvera Noah-Silvera enabled auto-merge (rebase) January 19, 2022 22:05
@Noah-Silvera
Copy link
Member Author

Just changed the fonts.googleapis blacklist to https!

@Noah-Silvera Noah-Silvera merged commit 32b1e50 into SuperGoodSoft:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarednorman jarednorman jarednorman approved these changes

@benjaminwil benjaminwil benjaminwil approved these changes

@forkata forkata forkata approved these changes

@nvandoorn nvandoorn Awaiting requested review from nvandoorn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Noah-Silvera @benjaminwil @jarednorman @forkata