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

Combine conftest.py files #2669

Merged
merged 10 commits into from
Feb 24, 2023
Merged

Combine conftest.py files #2669

merged 10 commits into from
Feb 24, 2023

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Feb 22, 2023

Closes #2174

Code Changes

  • Where possible combined the conftest.py files from ctl, lib, and ops into a single conftest.py file.

Steps to Confirm

  • All tests pass

Pre-Merge Checklist

Description Of Changes

Because of the difference in database connections between ctl and ops I was not able to combine those fixtures. In addition some of the tests in ctl seem to depend on previous tests having already run and populating the database with values. Because of this the fixture to reset data between tests could not be used in ctl.

There are some fixtures here that could be combined into one. I started down that path and realized there was going to be potentially a huge amount of files changed. Because of that I wanted to get this merged as a smaller change to make sure it was working, and can follow up with the larger change of merging duplicate fixtures.

@cypress
Copy link

cypress bot commented Feb 22, 2023

Passing run #361 ↗︎

0 3 0 0 Flakiness 0
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 8472ae0 into 72b5dfe...
Project: fides Commit: 7c1284bf4a ℹ️
Status: Passed Duration: 00:39 💡
Started: Feb 24, 2023 12:23 AM Ended: Feb 24, 2023 12:23 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@sanders41
Copy link
Contributor Author

The ctl tests are freezing because of a database deadlock. This originally happened when I tried keeping the database connections in one file. Splitting them out fix it, but after merging main it is unhappy again. I'm looking into the new cause/solution.

@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Base: 86.46% // Head: 86.46% // No change to project coverage 👍

Coverage data is based on head (8472ae0) compared to base (72b5dfe).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2669   +/-   ##
=======================================
  Coverage   86.46%   86.46%           
=======================================
  Files         289      289           
  Lines       15984    15984           
  Branches     2026     2026           
=======================================
  Hits        13821    13821           
  Misses       1779     1779           
  Partials      384      384           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sanders41
Copy link
Contributor Author

The ctl tests are freezing because of a database deadlock. This originally happened when I tried keeping the database connections in one file. Splitting them out fix it, but after merging main it is unhappy again. I'm looking into the new cause/solution.

Issue resolved.

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good to me! a few minor comments/questions on the code, nothing really that needs to be addressed as far as i can see.

looks like you've got a bunch of merge conflicts again with main that you need to resolve, sorry for not getting to this sooner :( but i'll approve this now preemptively.

a few other non-blocking notes:

  • should we create a follow up ticket for this?

can follow up with the larger change of merging duplicate fixtures

  • separately, should we run the unsafe test suite, just since you've touched things used by those tests and to make sure things aren't terribly broken? we can expect some intermittent failures on the saas external tests so we can use our discretion to ignore any failures there if they seem unrelated to your changes.

tests/ctl/conftest.py Show resolved Hide resolved
tests/lib/conftest.py Show resolved Hide resolved
tests/lib/conftest.py Show resolved Hide resolved
tests/lib/conftest.py Show resolved Hide resolved
@sanders41 sanders41 added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Feb 24, 2023
@sanders41
Copy link
Contributor Author

should we create a follow up ticket for this?

Yes, follow up created here #2688

separately, should we run the unsafe test suite, just since you've touched things used by those tests and to make sure things aren't terribly broken? we can expect some intermittent failures on the saas external tests so we can use our discretion to ignore any failures there if they seem unrelated to your changes.

Yes, makes a lot of sense. I set them to run this time.

@sanders41 sanders41 added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Feb 24, 2023
@sanders41
Copy link
Contributor Author

External tests are still getting skipped even after adding the label. I tried removing and re-adding it but that didn't kick them off either. Any other ideas on how to get them to run?

@sanders41 sanders41 added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Feb 24, 2023
@adamsachs
Copy link
Contributor

External tests are still getting skipped even after adding the label. I tried removing and re-adding it but that didn't kick them off either. Any other ideas on how to get them to run?

hmm not off the top of my head, pretty sure the removing and re-adding is always what i've done to trigger it when it's not cooperating... 🤔

@sanders41 sanders41 added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Feb 24, 2023
@sanders41
Copy link
Contributor Author

I ran the external tests locally and they failed as expected. All the failures look permissions related and not fixture related. I'm not sure of a better test I can do since I can't get them to start here. Thoughts?

@sanders41 sanders41 added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Feb 24, 2023
@adamsachs adamsachs added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Feb 24, 2023
@adamsachs
Copy link
Contributor

I ran the external tests locally and they failed as expected. All the failures look permissions related and not fixture related. I'm not sure of a better test I can do since I can't get them to start here. Thoughts?

just noting we synced up offline and i passed along the documentation about how to set up vault integration locally and @sanders41 is going to see if he can get that working. generally though we feel pretty confident this change is good to go.

@sanders41 sanders41 merged commit 65c2c64 into main Feb 24, 2023
@sanders41 sanders41 deleted the ps-conftest branch February 24, 2023 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor tests/ to use a single conftest.py
2 participants