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

Test on variety of pydantic versions 🩷 #503

Merged
merged 31 commits into from
Oct 18, 2024
Merged

Conversation

sydney-runkle
Copy link
Member

@sydney-runkle sydney-runkle commented Oct 15, 2024

Add tests with a pydantic versions matrix

Not sure if we need to deal with coverage here, happy to remove that part if we want.

Copy link

cloudflare-workers-and-pages bot commented Oct 15, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 2e5dc79
Status: ✅  Deploy successful!
Preview URL: https://08c8d4a0.logfire-docs.pages.dev
Branch Preview URL: https://try-pydantic-matrix.logfire-docs.pages.dev

View logs

@sydney-runkle sydney-runkle marked this pull request as ready for review October 15, 2024 19:06
@sydney-runkle sydney-runkle changed the title Adding pydantic versions matrix to tests Test on variety of pydantic versions 🩷 Oct 15, 2024
Copy link

codecov bot commented Oct 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (fa559ad) to head (2e5dc79).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #503   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          132       132           
  Lines         9986     10004   +18     
  Branches      1343      1349    +6     
=========================================
+ Hits          9986     10004   +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Kludex
Copy link
Member

Kludex commented Oct 16, 2024

Should we test OTel packages min/max as well? 🤔
Also, should we test everything, or just the pydantic plugin?

This job needs to be added to the check job btw.

@sydney-runkle
Copy link
Member Author

Good question. I'll defer to you and @alexmojaki re Otel. Will add to check when I'm back at my computer.

I think just testing the plugin makes sense given that pydantic isn't a dependency.

@alexmojaki
Copy link
Contributor

Not sure if we need to deal with coverage here, happy to remove that part if we want.

We have code like if get_version(pydantic.__version__) < get_version('2.5.0'): # pragma: no cover (not the only such case) so:

  1. Please keep the coverage stuff.
  2. Those pragmas should be removed
  3. Please test as far back as 2.4, and also one version of v1 (search the repo for pydantic v1)

Should we test OTel packages min/max as well? 🤔

We definitely should, in another PR

Also, should we test everything, or just the pydantic plugin?

Everything, just to be safe. These are fast. It's not just the plugin anyway, it's also about encoding pydantic objects as JSON.

Is there not a way to do this basically with just the GHA matrix, so we don't duplicate a bunch of stuff?

@sydney-runkle
Copy link
Member Author

Please keep the coverage stuff.

Done

Those pragmas should be removed

Done

Please test as far back as 2.4, and also one version of v1 (search the repo for pydantic v1)

Done

Everything, just to be safe. These are fast. It's not just the plugin anyway, it's also about encoding pydantic objects as JSON.

Sounds great, thanks for clarifying.

Should we test OTel packages min/max as well? 🤔

I've opened #507 to track this.

@sydney-runkle
Copy link
Member Author

Need to investigate failing coverage tomorrow. Not sure why that's the case now that we test on the variety of pydantic versions.

@sydney-runkle
Copy link
Member Author

Do we want to do the full cross product of python x pydantic versions here?

@alexmojaki
Copy link
Contributor

No, I think the cross product is extreme. I think https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow#expanding-or-adding-matrix-configurations should allow testing many pydantic versions on just 3.12.

Note that normally CI uses the latest git commit of pydantic, we still want to test that for all Python versions.

@alexmojaki
Copy link
Contributor

Looks like the coverage still isn't working. Once that's fixed, please search the repo for pydantic v1 and remove associated pragmas.

@sydney-runkle
Copy link
Member Author

It's not immediately evident to me why we're missing these lines on v1 testing.

@alexmojaki alexmojaki enabled auto-merge (squash) October 18, 2024 15:21
@alexmojaki
Copy link
Contributor

Thanks, sorry it was so painful!

@alexmojaki alexmojaki merged commit 46fee2b into main Oct 18, 2024
17 checks passed
@alexmojaki alexmojaki deleted the try-pydantic-matrix branch October 18, 2024 15:24
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.

3 participants