New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve tests and coverage for `localca.py` #1324

Merged
merged 3 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@plettich
Contributor

plettich commented Nov 27, 2018

Testing the create_ca() function with a predefined input string.
Also added more accurate descriptions.

Merging #1324 into master will increase coverage by 0.38%.
The diff coverage is 100%.
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
+ Coverage   96.01%   96.39%   +0.38%
==========================================
Files         141      141
Lines       17175    17175
==========================================
+ Hits        16490    16556      +66
+ Misses        685      619      -66
Impacted Files Coverage Δ
privacyidea/lib/caconnectors/localca.py 96.55% <100%> (+25.28%) ⬆️

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 4da61fa...64c6c6a. Read the comment docs.

  •  File: tests/test_lib_caconnector.py:L78-92
  • Nifty :) Maybe one could also do this with mock.patch, but I think this is nice already!
  • First i was looking into patching the input function but now i use mock to patch sys.stdin
  •  File: privacyidea/lib/caconnectors/localca.py:L569-583
  • I think the original idea was, that if we one day have other CA types like a MS CA, the create method could need some more information, this is why it was created as classmethod. (Well, actually the method is missing in the baseclass.
    But the the pi-manage ca create -t msca could also work.
    We can however change this and if necessary change this back, when we create an MSCA connector one day.
    What do you think?
  • I have been thinking about that as well.
    One idea would be to create some kind of factory function which returns a LocalCAConnector object.
    Currently the CAConnector objects are only instantiated inside the lib/caconnector.py module. Outside they are only described by their configuration.
    The create_ca() function only generates a suitable configuration which is then passed to save_caconnector() in lib/caconnector.py. This is also called by the caconnector api endpoint.
    So i think, create_ca() should not be part of the caconnector class, since, in this case, it creates the CA on disk. Would this be the same for the MS CA? I think this fits better in pi-manage or a separate script.
  • The localca.create_ca must not be part of pi-manage. It must be part of the localca module, since each connecter would have it's own logic of creating a CA setup. So this logic needs to be part of the module. Not sure if it needs to be part of the class - probably not.
    Currently I do not know, how the creation of a MS CA would look like. No idea, so we should stay flexible enough.
    staticmethod is totally fine for me, now.
  •  File: privacyidea/lib/caconnectors/localca.py:L596-605
  • There is a colon missing
    :return: The LocalCAConnector configuration

  

Improve tests and coverage for `localca.py`
Testing the `create_ca()` function with a predefined input string.
Also added more accurate descriptions.

@plettich plettich requested a review from privacyidea/core Nov 27, 2018

@codecov

This comment has been minimized.

codecov bot commented Nov 27, 2018

Codecov Report

Merging #1324 into master will increase coverage by 0.38%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
+ Coverage   96.01%   96.39%   +0.38%     
==========================================
  Files         141      141              
  Lines       17175    17175              
==========================================
+ Hits        16490    16556      +66     
+ Misses        685      619      -66
Impacted Files Coverage Δ
privacyidea/lib/caconnectors/localca.py 96.55% <100%> (+25.28%) ⬆️

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 4da61fa...f6e76b5. Read the comment docs.

@@ -73,6 +78,14 @@
"emailAddress=steve@openssl.org"
@contextmanager
def replace_stdin(target):

This comment has been minimized.

@fredreichbier

fredreichbier Nov 28, 2018

Member

Nifty :) Maybe one could also do this with mock.patch, but I think this is nice already!

This comment has been minimized.

@plettich

plettich Nov 28, 2018

Contributor

First i was looking into patching the input function but now i use mock to patch sys.stdin

Simplified test script
Instead of using a self-written context manager to replace `stdin` with a
string, the `mock` module is used to patch `sys.stdin`
@@ -593,7 +596,9 @@ def create_ca(cls, name):
* We create two templates for users and for servers.
:param name: The name of the CA connector.
:return:
:type name: str
:return The LocalCAConnector configuration

This comment has been minimized.

@cornelinux

cornelinux Nov 29, 2018

Member

There is a colon missing

:return: The LocalCAConnector configuration
@@ -73,6 +78,14 @@
"emailAddress=steve@openssl.org"
@contextmanager
def replace_stdin(target):

This comment has been minimized.

@cornelinux
@classmethod
def create_ca(cls, name):
@staticmethod
def create_ca(name):

This comment has been minimized.

@cornelinux
@@ -593,7 +596,9 @@ def create_ca(cls, name):
* We create two templates for users and for servers.
:param name: The name of the CA connector.
:return:
:type name: str
:return The LocalCAConnector configuration

This comment has been minimized.

@cornelinux

@cornelinux cornelinux merged commit bb1ed86 into master Nov 30, 2018

0 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@cornelinux cornelinux deleted the update_caconnector_test branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment