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

Send pytest coverage reports to code climate #2198

Merged
merged 54 commits into from
Jan 20, 2023

Conversation

ThomasLaPiana
Copy link
Contributor

@ThomasLaPiana ThomasLaPiana commented Jan 11, 2023

Closes #2179

Code Changes

  • refactor nox sessions to use a single parent pytest session that calls the other test functions
  • run isort(fix) and black(fix) first so that nox -s static_checks will try to fix files before checking them
  • separate the strictly pytest CI checks from the general backend checks to streamline the jobs that upload code coverage
  • update the test functions to write out coverage files based on their name
  • add a script that fixes the coverage files for Code Climate consumption
  • update CI jobs to upload those artifact files
  • send coverage reports to Code Climate in CI (CC Docs)
    • store each coverage report in a temporary storage destination (Github Artifacts)
    • pull them all in in a final job and send to Code Climate
  • add badges to the README

Steps to Confirm

  • coverage reports show up in Code Climate
  • safe pytest jobs produce and upload a coverage file (4 tests)

Pre-Merge Checklist

  • All CI Pipelines Succeeded (external checks are known-failing)
  • Documentation:
  • Issue Requirements are Met
  • Relevant Follow-Up Issues Created
  • Update CHANGELOG.md

Description Of Changes

Disclaimer: This change was meant to be relatively small, but in true "give a mouse a cookie" fashion it snow-balled into something much larger. Generally the changes are quite safe and is mostly all new functionality as opposed to modifying existing functionality.

Disclaimer 2: This change only handles uploading the coverage reports to Code Climate, there is still a bug preventing the code comment from updating on every push to the PR that will need to be solved in a PR for #2180

This PR adds the functionality that allows for continual tracking of overall code coverage via a 3rd party. This required a lot of engineering in terms of CI/scripting and has also touched how tests are run via nox sessions.

As a caveat, we are not including external test coverage as part of this work.

Nox changes

The nox suite of test sessions now looks like this:

- pytest(ctl-unit) -> Runs Pytests.
- pytest(ctl-not-external) -> Runs Pytests.
- pytest(ctl-integration) -> Runs Pytests.
- pytest(ctl-external) -> Runs Pytests.
- pytest(ops-unit) -> Runs Pytests.
- pytest(ops-integration) -> Runs Pytests.
- pytest(ops-external-datastores) -> Runs Pytests.
- pytest(ops-saas) -> Runs Pytests.
- pytest(lib) -> Runs Pytests.

Everything has been converted to a single pytest session with individual testing groups as a parameter.

@ThomasLaPiana ThomasLaPiana self-assigned this Jan 11, 2023
@ThomasLaPiana ThomasLaPiana marked this pull request as ready for review January 11, 2023 08:34
@codeclimate
Copy link

codeclimate bot commented Jan 11, 2023

Code Climate has analyzed commit 01719d8 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 71.0%.

View more on Code Climate.

@ThomasLaPiana ThomasLaPiana marked this pull request as draft January 11, 2023 08:36
@ThomasLaPiana
Copy link
Contributor Author

At this point I've gotten the coverage reports writing out successfully, and am now toying with the proper CI syntax to get everything to upload

One potential stumbling block is how to handle the fact that we only sometimes run the unsafe CI checks...I'm not sure yet if this will throw false positives about code coverage if those don't get run

@ThomasLaPiana
Copy link
Contributor Author

After some more testing and using this comment as an example, it appears to be an issue caused by running the tests within a docker file.

Going to try some things locally such as setting a specific source file and seeing if that helps things

@ThomasLaPiana
Copy link
Contributor Author

I noticed that it was saying that fides didn't exist in the python 3.8 python path, which seemed odd because I had specifically installed python 3.10. It dawned on me that maybe the third-party github action we're using wouldn't respect that, so I removed that CI task and want to see if it works just using the default python path

@ThomasLaPiana
Copy link
Contributor Author

ThomasLaPiana commented Jan 17, 2023

after adding some custom logic in the last commit, I got a valid report uploaded, hopefully it works in CI as well

EDIT: It broke in CI due to a file permissions error, will try fixes for that

@ThomasLaPiana
Copy link
Contributor Author

@sanders41 any idea on this error? It's a CI-specific one, and seems arbitrary

@sanders41
Copy link
Contributor

The 3 most common reasons I have seen for this are:

  1. It really is a permission issues. This one seems unlikely since you could write to it before. Could be ruled out by running with sudo
  2. Trying to open a directory instead of a file. It sure looks like a file to me, but maybe GitHub actions is doing something weird here and making a directory. Usually this would be debugged by checking that the path is a file before trying to open, but this should be the first creation of the file. May be worth trying though just to see what we get.
  3. Something else already has the file open.

@ThomasLaPiana
Copy link
Contributor Author

It seems like writing to files from within the docker container is perfectly fine, but trying to do it Pythonically within the nox session doesn't work...I guess my quick "fix" here would be to also do that part in the docker container

@ThomasLaPiana
Copy link
Contributor Author

The test reports are now working! https://codeclimate.com/repos/63be6d666de62000f3e44a88/settings/test_reports/63c7791b56b68a1d0b82b1a4

… coverage with less or more than 4 summed coverage results
@ThomasLaPiana ThomasLaPiana added the run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials label Jan 18, 2023
@ThomasLaPiana
Copy link
Contributor Author

The (for real this time though) final bug I've discovered is that the branch name in GitHub Actions is given as 2198/merge instead of ThomasLaPiana-send-coverage...

Due to this mismatch, the auto-updating code comment is not seeing what is actually happening in this PR and was instead only reflecting results from my local runs (where everything is working as intended)

I will update the CI checks with hopefully the proper github action var and should be set

@ThomasLaPiana
Copy link
Contributor Author

using the head ref appeared to fix the previous issue

image

@ThomasLaPiana
Copy link
Contributor Author

ThomasLaPiana commented Jan 18, 2023

using the head ref appeared to fix the previous issue

image

Even though it matches in the UI, it doesn't appear to be working because the comment still isn't updating...

I've decided to make #2180 it's own PR to tackle this directly and in a more direct way. This PR is stale and needs to get reviewed/merged before it balloons any more

@ThomasLaPiana ThomasLaPiana removed the do not merge Please don't merge yet, bad things will happen if you do label Jan 19, 2023
@ThomasLaPiana ThomasLaPiana requested review from sanders41 and a team January 19, 2023 14:13
noxfiles/ci_nox.py Outdated Show resolved Hide resolved
@ThomasLaPiana ThomasLaPiana merged commit 2dbc8fd into main Jan 20, 2023
@ThomasLaPiana ThomasLaPiana deleted the ThomasLaPiana-send-coverage-reports branch January 20, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fideops-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automate generation of a single number that represents total test coverage
2 participants