feat(BigQuery): Add reservation and jobTimeoutMs setters to JobConfigurationTrait#8685
feat(BigQuery): Add reservation and jobTimeoutMs setters to JobConfigurationTrait#8685cy-yun merged 1 commit intogoogleapis:mainfrom
Conversation
product-auto-label
Bot
added
the
api: bigquery
| * @param string $reservation The reservation to use. | ||
| * @return JobConfigurationInterface | ||
| */ | ||
| public function reservation(string $reservation) |
There was a problem hiding this comment.
Let's add the return type here:
public function reservation(string $reservation): JobConfigurationInterfaceThere was a problem hiding this comment.
When I add the return type and run the Unit tests I wrote, I get this failure
TypeError : Trait6e3f66f4623bd9942662da0d3da3875dc1f84673::jobTimeoutMs(): Return value must be of type Google\Cloud\BigQuery\JobConfigurationInterface, Trait6e3f66f4623bd9942662da0d3da3875dc1f84673 returned
So I omitted the return type. Let me know if you have any suggestions.
There was a problem hiding this comment.
This is because you're not returning JobConfigurationInterface, you're returning an instance of TestHelpers::impl(JobConfigurationTrait::class), which is mocking a trait class (e.g. Trait12345...).
So you need to either write the test in a way that actually returns a JobConfigurationInterface, or you need to change the return type to be something like self instead...
| * @param int $jobTimeoutMs The job timeout in milliseconds. | ||
| * @return JobConfigurationInterface | ||
| */ | ||
| public function jobTimeoutMs(int $jobTimeoutMs) |
There was a problem hiding this comment.
Return type here as well.
There was a problem hiding this comment.
Same response as above
|
We might want to add some tests for this. I would recommend writing a test that mocks the transport layer and checks that the new flag is in there as well as sending it on a System test to see if the server responds with the expected data. We cannot confirm the server behaviour at times, but just the fact of adding a test that sends the flag helps us keeping regressions to a minimum that being either from code changes from our side or the BigQuery team itself. |
|
This is a feature, as new surfaces are being added. It should be marked with |
Please take a look at the tests I've added |
cy-yun
force-pushed
the
reservation
branch
3 times, most recently
from
b4cefed to
eea52ce
Compare
| $this->assertFalse(isset($job->info()['status']['errorResult'])); | ||
| } | ||
|
|
||
| public function testLoadJobWithReservationAndTimeout() |
There was a problem hiding this comment.
Ran this on a test GCP account and confirmed that it passes.
cy-yun
changed the title
3 participants
https://buganizer.corp.google.com/issues/414633602