UWS error messages now sit on errorsummary. #432
Conversation
Previously, they were parsed into job.message, which was probably unintended, as there were message attributes on errorsummary that just were never filled because errorsummary would not parse its children itself. The alternative would be to preserve the previous behaviour, where message sits on the job element; but that's against the UWS schema, and if we did that, we'd have to take away the extra attributes on errorsummary (and revert the change to raise_if_error in the TAPJob). If we do it like this, that would fix Bug astropy#431.
msdemlei
force-pushed
the
fix-uws-errorsummary
branch
from
66b3cd4
to
3cac035
Compare
Codecov Report
@@ Coverage Diff @@
## main #432 +/- ##
==========================================
+ Coverage 79.81% 79.84% +0.03%
==========================================
Files 52 52
Lines 5989 5988 -1
==========================================
+ Hits 4780 4781 +1
+ Misses 1209 1207 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| if 'test_erroneus_submit.non_existent' in request.text: | ||
| job.phase = 'ERROR' | ||
| job.message = 'test_erroneus_submit.non_existent not found' | ||
| job._errorsummary = ErrorSummary() | ||
| job.errorsummary.message = Message() | ||
| job.errorsummary.message.content =\ | ||
| 'test_erroneus_submit.non_existent not found' |
There was a problem hiding this comment.
are these used anywhere in the tests? E.g. if I take the test changes from this PR and run them against the main branch, this doesn't trigger a failure (changes in pyvo/io/uws/tests/test_job.py do).
So this is somewhat unrelated and a more generic comment: I think building up complex mock classes but then don't fully use them in the tests are a bit misleading.
|
On Mon, Mar 06, 2023 at 09:42:18AM -0800, Brigitta Sipőcz wrote:
+ job.errorsummary.message.content =\
+ 'test_erroneus_submit.non_existent not found'
are these used anywhere in the tests? E.g. if I take the test
changes from this PR and run them against the main branch, this
doesn't trigger a failure (changes in
`pyvo/io/uws/tests/test_job.py` do).
That's odd -- are you sure? You see, this is scaffolding so
AsyncTAPJob.raise_if_error finds the error message; current main
expects it in self._job.message, this PR fixes it to be where we parse it
to now. I don't have time at the moment to investigate further, but
I have a hard time imagining how the code in main would find the
error message with this scaffolding.
So this is somewhat unrelated and a more generic comment: I think
building up complex mock classes but then don't fully use them in
the tests are a bit misleading.
Absolutely agreed. But then TAP is a complex beast, and if we want
some test coverage without remote services (and the fragility those
bring with them), I don't think we can do a lot better that what's
currently in test_tap (short of including a TAP engine, which at this
point is certainly not an option).
|
|
@bsipocz I can confirm it solves my issue. And the default message from |
Yeap, pulling only the changes from the test files to main, only one of them produces a failure, suggesting the change parts of the mock is not touched by any asserts.
Yes, that's understandable. I wonder maybe with some borderline trickery we could include these mock classes into the coverage run, which may help to smoke out the parts that are in the mock but not touched by the tests. Either case, solving or improving the situation points beyond this PR, so I go ahead and merge it given the confirmation above. |
|
Thanks @msdemlei for the fix! |
UWS error messages now sit on errorsummary.
|
On Tue, Mar 07, 2023 at 09:11:36AM -0800, Brigitta Sipőcz wrote:
Yeap, pulling only the changes from the test files to main, only
one of them produces a failure, suggesting the change parts of the
mock is not touched by any asserts.
FAILED pyvo/io/uws/tests/test_job.py::TestJob::test_error_job - AssertionError: assert None == 'We have problem'
Umm, I have a feeling I'm embarrassing myself again, but: this is the
test that the updated scaffolding "fixes"; it exercises picking the
error message from the UWS response in raise_if_error.
Are you concerned because by fidgeting with the scaffolding we don't
properly exercise the parsing component? Yeah, that's not pretty,
and I don't think any test properly does this at this point. Having
tests that have a few UWS responses and make sure everything is at
its place (with the right type) after parsing would probably be a
win, but building these would have required more time that I wanted
to invest in this fix (and we didn't have them before, either).
Or do I misunderstand what you're skeptical about?
|
Previously, they were parsed into job.message, which was probably unintended, as there were message attributes on errorsummary that just were never filled because errorsummary would not parse its children itself.
The alternative would be to preserve the previous behaviour, where message sits on the job element; but that's against the UWS schema, and if we did that, we'd have to take away the extra attributes on errorsummary (and revert the change to raise_if_error in the TAPJob).
If we do it like this, that would fix Bug #431.