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

fix: Separate temporal tests #19012

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Conversation

mcrumiller
Copy link
Contributor

@mcrumiller mcrumiller commented Sep 30, 2024

I'm not 100% sure why the new temporal literal tests were sometimes failing. It appears as though sometimes the to_datetime anyvalue converter was being called when the input was a time or timedelta.

My theory is that because the superclass types were parametrized in pytest, and because the tests are run in parallel, the MyClass classes were somehow colliding across threads. Thus, the MyClass(datetime) subclass defined in the test function would be defined in one thread while the lit(MyClass(time_input)) was being run in another, and the class ancestry lookup on the rust side would find the wrong superclass. I've split the tests and given the subclasses separate names so I hope this fixes it. However, all tests have passed before, so all tests passing now doesn't necessarily mean the issue is fixed.

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Sep 30, 2024
Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.87%. Comparing base (c23266b) to head (2207cc0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #19012      +/-   ##
==========================================
+ Coverage   79.84%   79.87%   +0.03%     
==========================================
  Files        1524     1524              
  Lines      207653   207653              
  Branches     2905     2905              
==========================================
+ Hits       165802   165868      +66     
+ Misses      41303    41237      -66     
  Partials      548      548              

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

@ritchie46
Copy link
Member

Right.. Sounds peculiar. But let's try it. 👍

@ritchie46 ritchie46 merged commit e402e70 into pola-rs:main Sep 30, 2024
15 checks passed
@mcrumiller mcrumiller deleted the fix-temporal-test branch September 30, 2024 10:56
@mcrumiller
Copy link
Contributor Author

Looks like this did not work, I'm still seeing failures as in here: https://github.com/pola-rs/polars/actions/runs/11106438782/job/30854771654?pr=19024.

I have work today but I can try to look more tonight, sorry about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants