MatterMiners / tardis

Transparent Adaptive Resource Dynamic Integration System
https://cobald-tardis.readthedocs.io
MIT License
14 stars 21 forks source link

Add error handling when updating non-existing record #306

Closed QuantumDancer closed 1 year ago

QuantumDancer commented 1 year ago

This is my proposed solution to fix #305

I tried to only catch this specific exception. All other exceptions should still be passed along.

Please let me know if we should do things differently.

Also, I was not sure how I can create a unit test for this specific case, because the auditor server is not directly mocked if I understood the test setup correctly.

I tested this in Freiburg in production and found no issues with this fix so far.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2bf9dcb) 98.84% compared to head (19ca892) 98.85%.

:exclamation: Current head 19ca892 differs from pull request most recent head e513aac. Consider uploading reports for the commit e513aac to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #306 +/- ## ======================================= Coverage 98.84% 98.85% ======================================= Files 56 56 Lines 2345 2356 +11 ======================================= + Hits 2318 2329 +11 Misses 27 27 ``` | [Files Changed](https://app.codecov.io/gh/MatterMiners/tardis/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners) | Coverage Δ | | |---|---|---| | [tardis/plugins/auditor.py](https://app.codecov.io/gh/MatterMiners/tardis/pull/306?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MatterMiners#diff-dGFyZGlzL3BsdWdpbnMvYXVkaXRvci5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

giffels commented 1 year ago

Thanks a lot for the contribution. Without checking any details yet, could you add a unittest for that use-case as well, please? You could potentially use cls.mock_auditorclientbuilder_patcher to mock the behaviour. I think you can basically use the side_effect method of the mock self.mock_auditorclientbuilder to throw the exception, when updating the record.

self.client.update.side_effect = RuntimeError(...)

run_async(
            self.plugin.notify,
            state=DownState(),
            resource_attributes=self.test_param,
        )

"plus check the result and add further tests here to test re-raising etc."

self.client.update.side_effect = None
QuantumDancer commented 1 year ago

Thanks for the instructions regarding the unit testing. I've now added tests that should cover all three cases. Please let me know, I should change something else.