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

DPL-402 Improve robustness of Pacbio::LibraryFactory error handling #753

Open
JamesGlover opened this issue May 31, 2022 · 0 comments
Open

Comments

@JamesGlover
Copy link
Contributor

JamesGlover commented May 31, 2022

Describe the Housekeeping
When making some other changes, Rubocop complained about:

spec/models/pacbio/library_factory_spec.rb:119:11: C: RSpec/SubjectStub: Do not stub methods of the object under test.
          allow(factory).to receive(:valid?).and_return true

This test appears to be designed to check for a belt and bracers approach, and ensure that if the initial factory validation fails to catch something, then problems downstream will not result in an inconsistent state. However:

  1. It doesn't check the scenario where the container associations fail to persist. In this case it appears that the library will still be persisted.
  2. It is a very artificially engineered test, it would make more sense to mock the return values from the saves of each of library and container_material in turn, and check that the other wasn't saved - a test that I believe would fail with the current code.

As a result I think a large re-evaluation of the error handling should be considered. I would suggest:

  1. Consider switching the library.save && @container_material.save to use save! instead. We're generally expecting the earlier validation to catch everything, so this should indeed be 'exceptional'
  2. If the saves fail, currently the errors will probably be eaten, save will return false, but errors will still be blank, following the original save. This could be quite frustrating.

The test also doesn't consider other, perhaps more likely, error scenarios, such as database connection troubles, or index constraint violations. We should expect the factory to be transactional, and would need to evaluate the best way of handling these errors. (Ie. What is appropriate to return to the users, what should we be notified about)

Blocking issues
Describe any other issues or tickets that may be blocking this change.

Additional context
Add any other context about the problem here.

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

No branches or pull requests

1 participant