DiscipleTools / disciple-tools-web-components

https://jade-chebakia-17493f.netlify.app/?path=/story/kitchen-sink--kitchen-sink
GNU General Public License v2.0
3 stars 8 forks source link

Mocking API call in number test. #52

Closed incraigulous closed 1 year ago

incraigulous commented 1 year ago

The number field API calls needed to be mocked to pass now that we added error testing. I also noticed the tests failed when a value wasn't set, so I set values to get the tests to pass.

netlify[bot] commented 1 year ago

Deploy request for jade-chebakia-17493f rejected.

Name Link
Latest commit 8fd9f14587dda627977020fa59bb53569c92b810
incraigulous commented 1 year ago
Screenshot 2023-03-30 at 11 34 58 AM
incraigulous commented 1 year ago
Screenshot 2023-03-30 at 11 38 31 AM

@micahmills , I'm not sure why we're getting this error. I can run npm install and npm ci locally without issue.

incraigulous commented 1 year ago

There was no mocking in place (as far as I could tell). I added sinon.js and mocked the call that was failing. It was failing because we’re doing response status code checking on the number field now, so 404s were allowed before, but not anymore.

If we add API error checking to the rest of the fields, we’ll need to mock those, too, so you can replicate what I did with the number field test.

Using import map overrides to mock every API call when testing is also possible. Please take a look at the test runner docs on that. I did that at first but moved my mock to the test because mocking the entire API was a bit heavy-handed in case some tests want to test to failure.

cairocoder01 commented 1 year ago

Sorry to interject a little late on this, but it seems like the number field now auto-saves to the API all the time. That would prevent it from being used on a standard form that would just submit the field normally (such as the magic links plugin). Shouldn't there be something to not use the API to save automatically if doesn't have an apiRoot property or something? That could help some of the testing issues as well so you don't have to mock HTTP requests.

I had stubbed out the ComponentService a while back as a solution to optionally adding the auto-save feature to fields on-demand. You could call (new ComponentService()).enableAutoSave(selector) when a page loads to attach to all of the change events of the fields on the page and make the API calls separately when the change event is triggered. I think it will be easier to keep that functionality separate to avoid some of the issues above regarding testing when the API calls are baked in to the components. This number field seems to be the first one to add the API saving, so it might be worth having a discussion about which direction we want to go for enabling the API integration

incraigulous commented 1 year ago

@cairocoder01 should we close this out or modify it? You never know when we'll have a need for mocking, even if we aren't using the API, but I don't really see a use for it now.

cairocoder01 commented 1 year ago

Mocking could be useful, but I say we close this out and just add it in when we really need it