Skip to content
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

Add unit tests and new functional tests #64

Closed
wants to merge 17 commits into from
Closed

Conversation

larsks
Copy link
Member

@larsks larsks commented Jan 11, 2022

This is the third in a series of pull requests (of which #62 is the first and #63 is the second).


This PR is all about tests:

  • It adds a new suite of functional tests that are in general more granular and provide better information in the event of failure.
  • It adds unit tests for all major modules. This PR does not include 100% test coverage, but should provide a model for how to write additional tests. I have intentionally skipped writing tests for the rolebinding methods because of function MocOpenShift::update_user_role_project needs refactoring #36.
  • It updates the CI workflow to run the unit tests for each pr.

larsks added 17 commits January 11, 2022 16:14
By moving application creation into the create_app() method, we are
able to isolate application instantiation from the import of the wsgi
module. This is going to be necessary for writing unit tests, because
we will often want to mock out some top-level objects that can only be
modified after the module has been imported.

The pattern used in this PR is called an "application factory", and is
explained in more detail in [1].

[1]: https://flask.palletsprojects.com/en/2.0.x/patterns/appfactories/
Instead of creating a new MocOpenShfit4x instance in each handler
function, just create one when we start up the app. This allows us to
drop the MocOpenShiftSingleton wrapper.

x-branch: feature/application-factory
Moves all the logic for communicating with the Kubernetes API into the
kubeclient module. This replaces the get_request, del_request,
put_request, and post_request methods of MocOpenShift with calls to
the session object, which takes care of things like setting the
appropriate headers.

This also enables SSL certificate validation by default when run in
Kubernetes.
As we add additional files to the project it becomes inconvenient to
constantly modify the Dockerfile to include them. This updates the
Dockerfile to copy everything in the current directory, and adds a
.dockerignore file to prevent things that change regularly (like the
.git directory) from invalidating the cache.

NB: when building locally this will pick up anything in the current
working directory. This isn't an issue when building in CI or via the
automatic build features of quay.io or docker hub.
Use the config attribute of the flask app to aggregate configuration
from defaults, explicitly provided parameters, and the environment.
This provides a consistent mechanism for accessing values, and makes
it easier to pass in explicit config information when writing tests.
This adds an AUTH_DISABLED configuration item to the flask app which
allows us to disable authentication. We can use this during
development to make it easier to test the service, and we can use it
during testing to create a test client that doesn't require
authentication with every call.
By using a configmapGenerator [1] to generate the quota ConfigMap, we can
store the example data itself as a plain JSON file, which makes it
available to the code when running things locally.

[1]: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/kustomization/#configmapgenerator
Using `sleep` as a synchronization mechanism is problematic -- it
can both introduce unnecessary delays and result in unexpected
problems if an operation takes longer than expected.

This commit replaces the use of `sleep` with explicit polling.
Move all the account management code into the acct_mgt package.
Putting the code into a single namespace like this makes it easier to
write modular code, keeps the top-level directory organized, and makes
testing simpler.

x-branch: feature/prepare-for-tests
This introduces a new suite of functional tests. The tests are moved
into the `tests/functional` directory, which will allow us in
subsequent commits to place unit tests in `tests/unit`.
This adds a suite of unit tests for the flask application and modifies
the CI workflow to run unit tests in addition to the pre-commit
checks.
Several tests are marked with @pytest.mark.xfail() to indicate places
where the existing API may be problematic. For each of these tests, we
need to determine if the existing behavior is correct, and if it is,
modify the test so that it tests the current behavior and passes.
Add a GitHub workflow to run unit tests for each push and
pull-request.
- Add pytest-coverage to test-requirements.txt, which allows us to
  produce coverage reports.

- Update the CI workflow to include coverage reporting, which looks
  like this:

      Name                        Stmts   Miss  Cover
      -----------------------------------------------
      acct_mgt/__init__.py            0      0   100%
      acct_mgt/app.py               141     10    93%
      acct_mgt/defaults.py            2      0   100%
      acct_mgt/kubeclient.py         33     19    42%
      acct_mgt/moc_openshift.py     299    133    56%
      acct_mgt/wsgi.py                7      7     0%
      -----------------------------------------------
      TOTAL                         482    169    65%

- Add a note to the README about running unit tests.

x-branch: feature/tests
@larsks larsks closed this Jan 11, 2022
@larsks
Copy link
Member Author

larsks commented Jan 11, 2022

You can see the output of running the unit tests here.

@larsks
Copy link
Member Author

larsks commented Jan 11, 2022

There are currently several unit tests that are marked xfail (that is, expected to fail) because the behavior they test differs from the current implementation. Part of a future discussion would be if we want to remove the xfail tests or change the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant