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.

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

Test our use of ProgressEvent for form attachments #127

Closed
matthew-white opened this issue Aug 27, 2018 · 2 comments
Closed

Test our use of ProgressEvent for form attachments #127

matthew-white opened this issue Aug 27, 2018 · 2 comments
Labels
testing Integration tests, unit tests

Comments

@matthew-white
Copy link
Member

We don't currently have an easy way to mock ProgressEvent objects.

@matthew-white matthew-white added the testing Integration tests, unit tests label Jul 20, 2020
@matthew-white
Copy link
Member Author

This inability to test ProgressEvents is coming up again in getodk/central#589. To test them end-to-end would require modifying mockHttp(). However, mockHttp() is already fairly complex, so I'm reluctant to add this functionality to it. We use ProgressEvents in pretty simple ways, so I think it's enough to test them in unit tests and other ways.

In getodk/central#589, I'm writing a small utility function to convert a ProgressEvent to a fraction (the fraction of the total that's been uploaded). At that time, I'll write unit tests of the function. There will also be a new EntityUploadPopup component that will receive a ProgressEvent as a prop, and I'll write tests of that component on its own. The main thing that will be untested is the onUploadProgress function that we will pass to axios in EntityUpload. However, as in FormAttachmentList, that function is one line long. I think it's enough just to verify it manually.

I think I didn't take quite the right approach in testing FormAttachmentList, in that all its tests involve the top-level FormAttachmentList component and use load(). Child components are not tested in isolation, including FormAttachmentPopups, FormAttachmentRow, and others. If the component tests were more isolated, it'd be easy to test the effect of passing a ProgressEvent to FormAttachmentPopups.

I'm not eager to refactor the FormAttachmentList tests anytime soon: some of them are fairly complicated. We could think about breaking up the tests if/when we have additional work to do on FormAttachmentList. After breaking up the tests, we'd be able to test FormAttachmentPopups in isolation. That said, I don't think we need to keep this issue open indefinitely. I think we have a reasonable strategy going forward for testing ProgressEvents and can return to FormAttachmentPopups if/when it makes sense to do so.

@matthew-white
Copy link
Member Author

Funnily enough, we actually had a bug related to upload progress that I think was introduced very recently, in v2023.5: getodk/central#606. However, the bug stemmed from a change in axios itself, so it's not something that we would have caught using mockHttp(), which mocks axios. That sort of change to axios should be quite rare, so I think my comments above still apply. I think manual testing or maybe true end-to-end testing is the best way to catch a change to axios itself.

In getodk/central#589, I'm writing a small utility function to convert a ProgressEvent to a fraction

axios now calculates this fraction itself, so this utility function won't be needed. Because axios now does the calculation, our code around upload progress can become even simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Integration tests, unit tests
Projects
None yet
Development

No branches or pull requests

1 participant