Closed celestian closed 7 years ago
Merging #218 into master will increase coverage by
2.49%
. The diff coverage is97.04%
.
@@ Coverage Diff @@
## master #218 +/- ##
==========================================
+ Coverage 70.25% 72.74% +2.49%
==========================================
Files 37 41 +4
Lines 4027 4433 +406
Branches 442 449 +7
==========================================
+ Hits 2829 3225 +396
- Misses 1044 1052 +8
- Partials 154 156 +2
Impacted Files | Coverage Δ | |
---|---|---|
tests/functional/test_container.py | 100% <100%> (ø) |
|
tests/functional/test_key.py | 100% <100%> (ø) |
|
tests/functional/test_basics.py | 100% <100%> (ø) |
|
tests/functional/base.py | 86.36% <86.36%> (ø) |
|
src/custodia/client.py | 80.29% <0%> (+0.49%) |
:arrow_up: |
src/custodia/server/config.py | 77.88% <0%> (+0.96%) |
:arrow_up: |
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 0dccc81...4724477. Read the comment docs.
I sent fixed patch. All comments were addressed.
It failed on
CustodiaHTTPSTests.test_client_no_ca_trust
andCustodiaHTTPSTests.test_client_no_client_cert
what is addressed by #220
I push rebased version.
I did rebase of master into branch of this PR and I addressed all comments. Just two notes:
test_who_watches_the_watchers
as reaction on standard message Quis custodiet ipsos custodes?
which is sent by Custodia if you ping it. OK, tests are serious things---no room for jokes;DEFAULT_HEADERS
to value Content-Type': 'application/octet-stream
in client.py
.I would like to know your opinion to this point---one possible solution is rewrite constructor of CustodiaHTTPClient
in this way:
class CustodiaHTTPClient(object):
def __init__(self, url, headers=DEFAULT_HEADERS):
Does it looks good to you? Of course, this is not proper solution, becasue lint
said
Dangerous default value DEFAULT_HEADERS (__builtin__.dict) as argument (dangerous-default-value)
.
OK, I just pushed new version because I did little refactor.
I addressed all comments. And I merge master to this PR. (Wau, github has new feature for this, very nice and useful.)
Note: Actually I am working on tests for auth. plugins... it is possible that I will change functional/base.py
.
Yes, the new github merging tool is nice... but history wasn't nice. So I did it again.
To my comment about functional/base.py
:
In this PR, the situation is clear. I used pytest fixtures to prepare configuration and another one for starting Custodia server.
In new tests, which I am writing---the situation is more complicated. We can have parametrized tests, but it is imposibly to have parametrized fixtures. And I would like to test all possible configuration for every plugin.
I discussed this topic with others---I will use parametrized tests and all the configuration stuff and the start of the server will do special context manager for me.
Of course, this conversation we will do in new PR :-) It is just preview what I am thinking about.
I'm going to release a new version of Custodia today. We can do another release as soon as you have added more function tests.
There are set of simple functional tests. It is neccesary to run it on box with Custodia installed.
Signed-off-by: Petr Čech pcech@redhat.com