Tasks create http task with token #1112
Conversation
googlebot
added
the
cla: yes
MartinBarriga
force-pushed
the
tasks-create-http-task-with-token
branch
from
de78986 to
78d3721
Compare
tasks/test/tasksTest.php
Outdated
| private static $queue; | ||
| private static $location; | ||
| private static $url; | ||
| private static $email; | ||
| private static $payload; | ||
| private static $haveVariablesBeenInitialized = false; |
There was a problem hiding this comment.
Can you remove these and go back to relying on $this->requireEnv and the logic in TestTrait to set variables?
There was a problem hiding this comment.
It stills relies on $this->requireEnv and the logic in TestTrait to set the values of queue, location and projectId. Url, email and payload are variables that don't have the logic in TestTrait for getting their values, that's the reason of inserting their values manually in the code. The assignment of the values to each variable is made in initializeVariables(), and uses $this->requireEnv for queue and location, projectId isn't involved here because it's a static variable that is created and has its value assigned in TestTrait.
Should I try to look at a way to modify TestTrait so it can handle url, email and payload?
The current tasksTest code handles url and payload the same way as we do: https://github.com/GoogleCloudPlatform/php-docs-samples/blob/master/tasks/test/tasksTest.php
There was a problem hiding this comment.
Ok, I see, I guess we still need $queue and $location here then but I think it's fine to remove $url, $email and $payload variables and just hardcode the values where they are needed in each test case like the original version did.
There was a problem hiding this comment.
I agree with Jenn. I think it is better to take out url, email, payload from the class. Another reason is that it is likely we will have more tests that use different values for those. For example, a failure case where the url is broken or an invalid email, etc.
There was a problem hiding this comment.
Yeah, you're right. I'm moving $url, $email and $payload to be like in the original version to avoid failures.
tasks/test/tasksTest.php
Outdated
| self::$payload = 'Task Details'; | ||
| } | ||
|
|
||
| private function getExpectedOutput() |
There was a problem hiding this comment.
Maybe rename to getExpectedTaskName to make it more clear what it does? Or just inline this logic in the tests
There was a problem hiding this comment.
Yes, I need to be more specific with my functions names. What does inline the logic means?
There was a problem hiding this comment.
Great, that's a much clearer name. By inline I just meant including the line in each test case rather than breaking it out into a method.
tasks/test/tasksTest.php
Outdated
| return ob_get_clean(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry, I don't know if I got this, did you mean to add newline between the braces on line 100 and 101, right?
There was a problem hiding this comment.
Sorry, missed this the other day. I meant at the end of the file after the last brace not between the two braces.
There was a problem hiding this comment.
I made a new commit deleting the newline between the two braces and adding it at the last brace, but this newline at the end it's not being detected by github.
tasks/test/tasksTest.php
Outdated
| if(!self::$haveVariablesBeenInitialized){ | ||
| $this->initializeVariables(); | ||
| self::$haveVariablesBeenInitialized = true; | ||
| } |
There was a problem hiding this comment.
revert this block and the similar one below (see above comment)
There was a problem hiding this comment.
Sorry, if we remove this block, declare and initialize the variables in each of the test functions instead of having them as class static attributes, wouldn't we being doing a little bit of extra work?
For example, if we had n testing functions, instead of declaring and initializing the variables only once because they're static attributes and because we are making sure only executing initializeVariables() one time, we would be creating and assigning values n times, because there are n testing functions.
There was a problem hiding this comment.
Ok, makes sense, my original suggestion was to copy the initialization logic into each test case since there's only two right now but I think the better solution would actually be to use a setup method (similar to what you've done here with initializeVariables but built into the testing framework so you don't need to keep track of calling it in each case or ensuring it is only called once).
I'm not very familiar with php but it looks like there is a setUpBeforeClass method (doc: https://phpunit.de/manual/6.5/en/fixtures.html, example: https://github.com/GoogleCloudPlatform/php-docs-samples/blob/master/appengine/flexible/tasks/test/createTaskTest.php#L35) in phpunit that does what you want.
There was a problem hiding this comment.
Amazing, I'm implementing the setUpBeforeClass, seems to work well and now the code looks way more clean, thanks
tasks/test/tasksTest.php
Outdated
| return ob_get_clean(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorry, missed this the other day. I meant at the end of the file after the last brace not between the two braces.
MartinBarriga
force-pushed
the
tasks-create-http-task-with-token
branch
from
e937686 to
411d11f
Compare
|
@bshaffer and @SurferJeffAtGoogle would either of you be willing to review this PR? Thanks |
|
@grayside can you review this PR? |
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
Overall looks solid. I've commented to double check a minor practice concern.
7 participants
add php snippet and test for creating a http task with token