fix(job): check for errorResult when polling jobs

[callmehiphop]

Fixes #329

Would like some confirmation from a BQ team member on this one.

• Tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

[codecov]

Codecov Report

Merging #387 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #387   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files           4        4           
  Lines         544      544           
  Branches       75       75           
=======================================
  Hits          541      541           
  Misses          2        2           
  Partials        1        1

Impacted Files	Coverage Δ
src/job.ts	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5721fd1...3284913. Read the comment docs.

[stephenplusplus]

It does seem correct to use errorResults, considering these definitions:

status.errorResults

Final error result of the job. If present, indicates that the job has completed and was unsuccessful. For more information, see troubleshooting errors.

status.errors[]

The first errors encountered during the running of the job. The final message includes the number of errors that caused the process to stop. Errors here do not necessarily mean that the job has completed or was unsuccessful.

Although, I wonder what type of error could happen in status.errors[] that wouldn't be means for the job to be unsuccessful? That seems like it would be more of a warning as opposed to an error.

[callmehiphop]

@stephenplusplus so while investigating #329 I learned that you can set a maxBadRecords option while uploading a CSV file. Essentially you can have a certain number of erroneous rows and for each error an object will appear in the errors array. If you come in under the maxBadRecords value then errorResult will not be present and the job is considered a success.

[bcoe]

If I'm understanding this correctly, in the past if we received an error and a response we preferred the response? now error takes precedence?

This looks good to me; I'd probably lean towards calling it a breaking change? (wouldn't be shocked if people relied on the old behavior).

[bcoe]

nit: it would be cool if we could get a test case around this change in behavior.

[callmehiphop]

I actually thought this was a behavior change that wasn't pulled over to BigQuery correctly, but now that you said this I think we might have stumbled on to a bug in nodejs-common

[callmehiphop]

Oh, nevermind, my initial guess was right. There was a breaking change in common 0.28.0 and the fix apparently never found its way here.

[tswast]

Change to check for errorResult instead of any errors LGTM.

[callmehiphop]

@bcoe somehow I missed your comment, but the response can contain 2 different error fields. errorResult and errors. As it is today we check for errors and assume the job failed. I just learned that errors can in some cases serve more as a warning and that errorResult is more correct in determining if the job failed. When both errors and errorResult are present, the first item in errors (array) is actually the same value as errorResult, so users will still get the same exact error.

[callmehiphop]

@tswast so this error started popping up today

No matching signature for operator < for argument types: STRUCT<seconds INT64, nanos INT64>, TIMESTAMP. Supported signatures: ANY < ANY

Is there something going on with the backend, or should we be preparing a fix for this??

[tswast]

This looks like a problem in the back end handling of query parameters, unrelated to this PR. Thanks for the heads up. Investigating.