Skip to content

10787: Extract hook from Full Address Search#9163

Merged
allthesignals merged 2 commits intomainfrom
wmg/10787-fas-combine
Sep 8, 2023
Merged

10787: Extract hook from Full Address Search#9163
allthesignals merged 2 commits intomainfrom
wmg/10787-fas-combine

Conversation

@allthesignals
Copy link
Contributor

🎫 Ticket

🛠 Summary of changes

Partly addresses the ticket by extracting the hook into a separate file. This will make it easier to combine components later.

📜 Testing Plan

  • Behavior should remain the same as before. This is a small refactor.

@allthesignals allthesignals requested review from a team and tomas-nava September 7, 2023 17:36
@aduth
Copy link
Contributor

aduth commented Sep 7, 2023

Seems like it'd be a good opportunity to add some test coverage?

@gina-yamada
Copy link
Contributor

Sorry for the slow approval! I just got my local env working to test this out. Behavior working as before- was able to hit usps locally and get display, form validation working - LGMT

@allthesignals
Copy link
Contributor Author

Seems like it'd be a good opportunity to add some test coverage?

Sure. There is coverage for this feature but I could add a unit test for the hook, unless I'm missing something else

@allthesignals
Copy link
Contributor Author

Seems like it'd be a good opportunity to add some test coverage?

Also have we ever had a way to measure code coverage on this project? A CI report would be great.

@aduth
Copy link
Contributor

aduth commented Sep 8, 2023

Also have we ever had a way to measure code coverage on this project? A CI report would be great.

We have test coverage between CodeClimate (example) and GitLab "coverage" step (example), but I don't know that this has given an accurate representation of coverage of anything other than Ruby code. I think it'd be a good idea to see if we could extend this.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests 🎉

@allthesignals allthesignals merged commit 76e1df6 into main Sep 8, 2023
@allthesignals allthesignals deleted the wmg/10787-fas-combine branch September 8, 2023 16:48
@allthesignals
Copy link
Contributor Author

Also have we ever had a way to measure code coverage on this project? A CI report would be great.

We have test coverage between CodeClimate (example) and GitLab "coverage" step (example), but I don't know that this has given an accurate representation of coverage of anything other than Ruby code. I think it'd be a good idea to see if we could extend this.

Happy to look into this. Seems like there's some configuration alluding to JS in Code Climate YAML file, but the dashboard itself doesn't report anything about the JS.

@aduth aduth mentioned this pull request Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants