| kparal | |
| tflink | |
| mkrizek | |
| pschindl |
Initial version of the ResultsDB reporting directive. Once again, this is dependent on the kparal's Config.
Key points to adress:
None so far
| No Linters Available |
| No Unit Test Coverage |
Test coverage is being worked on :)
| libtaskotron/check.py | ||
|---|---|---|
| 299 ↗ | (On Diff #93) | Yup, there is a slight change to the code here. (lines 319 to 327), and I was not sure about the proper work flow. |
| libtaskotron/directives/resultsdb_directive.py | ||
|---|---|---|
| 20 | missing module docstring or at the very least a class docstring | |
I removed the CheckDetail related changes, and added tests.
I have two issues with the code though:
getting the 'resultsdb_job_id' - I guess that this is something the scheduler/runner should provide, once the resultsdb-related functionality is there. What is the best action now (apart of creating a ticket for it) - leave it like that, or create a temporary resultsdb job, and hardcode the job's id for the moment?
Can't the directive create the resultsdb job or are we assuming that'll be created at scheduling time?
getting the log urls - I guess this is a question for tflink mostly - where can I search for the (maybe yet non-existent) URL? Will there be some algorithmic solution, or will this need to be passed from the scheduler?
The log urls will be figured out algorithmically from the buildbot job id which needs to be part of the environment in the runner
Overall the code looks good. I have some concerns about the test code, comments listed inline
| libtaskotron/directives/resultsdb_directive.py | ||
|---|---|---|
| 64 | please try not to use really short variable names like this and the o - it tends to make code more difficult to read | |
| testing/test_resultsdb_directive.py | ||
| 12 | I don't understand why you're extending dingus here. As is, you're not really using any functionality from it. Why not use dingus or create a plain stub? | |
| 47 | This will hit the filesystem and adds some complexity that I don't think are really needed. I think it would be better to either stub/mock out the config object | |
Can't the directive create the resultsdb job or are we assuming that'll be created at scheduling time?
IIRC, we agreed that the job will be created at scheduling time, as the job represents the 'task life-cycle'. But at the moment, we can spawn the job in the reporting directive, and just switch to creating it at the scheduling time in the (not that far) future.
The log urls will be figured out algorithmically from the buildbot job id which needs to be part of the environment in the runner
Cool, I'll leave it be right now, and we can fix it later. I'll write a ticket ASAP.
| testing/test_resultsdb_directive.py | ||
|---|---|---|
| 12 | At the begining, I intended to check the call-stack at the end, to make sure no other methods were called. I can, of course, make it just a plain object with just that method, but I need to check that the arguments are passed properly, so plain Dingus instance IMHO won't be sufficient. But I'm more than open to suggestions, so if there is any obvious way, that I'm missing, feel free to point it out to me. | |
| 47 | I guess monkeypatching the config.get_config with something makes more sense. I just thought (maybe I just misunderstood), that the config module won't be touching filesystem by default. | |
| libtaskotron/directives/resultsdb_directive.py | ||
|---|---|---|
| 64 | Seconded. | |
| testing/test_resultsdb_directive.py | ||
| 12 | Please add a docstring, at least. | |
| 47 | Under py.test config.get_config() returns TestingConfig, which is not loaded from disk at all. However, Josef showed me some printouts about creating taskotron-related dirs: .[libtaskotron:config.py:80] 2014-04-10 10:33:16 DEBUG Not loading config files from disk as requested DEBUG:libtaskotron:Not loading config files from disk as requested [libtaskotron:config.py:61] 2014-04-10 10:33:16 DEBUG Using config profile: testing DEBUG:libtaskotron:Using config profile: testing [libtaskotron:config.py:237] 2014-04-10 10:33:16 WARNING Failed to create required directory: /var/cache/taskotron Cause: [Errno 13] Permission denied: '/var/cache/taskotron' WARNING:libtaskotron:Failed to create required directory: /var/cache/taskotron Cause: [Errno 13] Permission denied: '/var/cache/taskotron' [libtaskotron:config.py:237] 2014-04-10 10:33:16 WARNING Failed to create required directory: /var/run/taskotron Cause: [Errno 13] Permission denied: '/var/run/taskotron' WARNING:libtaskotron:Failed to create required directory: /var/run/taskotron Cause: [Errno 13] Permission denied: '/var/run/taskotron' [libtaskotron:config.py:237] 2014-04-10 10:33:16 WARNING Failed to create required directory: /var/log/taskotron Cause: [Errno 13] Permission denied: '/var/log/taskotron' It's true that _create_dirs() is executed even under py.test. It touches the disk, but if you have the directories in place, it just checks for existence. I can of course change that and don't run it when testing. My thinking was that we might need those directories present for certain functional tests. What do you think? (Also, a minor improvement might be to remove some dirs definitions until we really use them, like cachedir or lockdir). | |
At the begining, I intended to check the call-stack at the end, to make sure no other methods were called. I can, of course, make it just a plain object with just that method, but I need to check that the arguments are passed properly, so plain Dingus instance IMHO won't be sufficient.
An example of doing the same testing with dingus instead of a custom stub object:
{F1595}
An example of doing the same testing with dingus instead of a custom stub object:
Cool! Thanks for the tip. I'll change that shortly. Anything else in the current diff, that could use some fine-tuning?
Cool! Thanks for the tip. I'll change that shortly. Anything else in the current diff, that could use some fine-tuning?
Nope, the rest of it looks good to me
missing module docstring or at the very least a class docstring