fix: QueryJob.exception() *returns* the errors, not raises them

[plamut]

Fixes #451.

This PR makes sure that QueryJob.exception() actually returns exceptions as documented, not raises them.

Checklist:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[tswast]

Thanks!

[tswast]

Retracting approval until

Or can we call set_exception and return False in this case? We'd want a unit test that confirms result() doesn't get into an infinite loop.

is handled.

[plamut]

Hmm, TestQueryJob.test_result_w_retry seems to be flaky (failed run).

oogle.api_core.exceptions.RetryError: Deadline of 0.0s exceeded while calling functools.partial(functools.partial(<MagicMock name='mock.api_request' spec='function' id='140372364135632'>, method='GET', path='/projects/project/jobs/JOB_ID', query_params={}, timeout=None))

The same test passed with Python 3.6, 3.8, and also locally - let's see if the results are consistent on a re-run.

[plamut]

I was able to reproduce flakiness on my machine by reducing the retry deadline in affected tests by ~25 %, which means the latter was indeed the culprit. Because done() now does a bit of extra work, it appears that was just enough of a slowdown to start causing the failures (at least under Python 3.7).

Although not ideal, increasing the deadline should be a solution sufficient with high enough probability.

[tswast]

why use the private property in this and the other tests? any objections to calling job.exception() here and the other tests?

[plamut]

The reasoning was that job.exception() can execute additional logic, and errors in that method would make the tests for done() fail, too, even if there was nothing wrong with the done() method itself.

One could argue that the chosen unit of test is too small, and that the class itself should represent a unit as opposed to its individual methods, but addressing that would require quite some refactoring (we already tinker with internal _properties , for instance).

Here, practicality beats purity IMHO, thus the "cheating" by examining the internal state of the class. Or do you have a strong opinion on this?

[tswast]

Works for me. I agree that ideally we'd have higher-level tests than this, but makes sense to stay with existing conventions, especially given our 100% coverage requirement.

[tswast]

Works for me. I agree that ideally we'd have higher-level tests than this, but makes sense to stay with existing conventions, especially given our 100% coverage requirement.