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 timezone issue in unittest test_naive_date_time_param and test_da… #150

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jason-ni
Copy link

Happen to find 2 UT failures after cloned. It seems the timestamp without time zone in DuckDB is converted to local time omitting the time zone information while with uint64 seconds number conting from Epoch as UTC.

@wangfenjin
Copy link
Collaborator

Seems clippy add new rules, you can run
cargo clippy --fix --allow-dirty --all-targets --workspace --all-features -- -D warnings -A clippy::redundant-closure in your local folder

@jason-ni
Copy link
Author

I don't see that clippy warning locally...

@wangfenjin
Copy link
Collaborator

@jason-ni you can rebase master, I had fix the issue

@jason-ni
Copy link
Author

In the latest main branch, I still see failures in my local environment. I guess your development os time is set to UTC, is it?

cargo test --features buildtime_bindgen --features modern-full -- --nocapture
......
test types::test::test_dynamic_type ... ok
test types::to_sql::test::test_uuid_gen ... ok
test types::test::test_option ... ok
test types::to_sql::test::test_uuid_type ... ok
test types::to_sql::test::test_uuid_blob_type ... ok
test types::url::test::test_sql_url ... ok
test test::test_query_arrow_record_batch_large ... ok

failures:

failures:
    types::chrono::test::test_date_time_param
    types::chrono::test::test_naive_date_time_param

test result: FAILED. 112 passed; 2 failed; 14 ignored; 0 measured; 0 filtered out; finished in 0.88s

@wangfenjin
Copy link
Collaborator

shell > date
Thu Apr 27 10:27:27 CST 2023

It's my local time. From my understanding it shouldn't have issue, as the github CI always work. But you can just rebase the master and if CI also pass then I think your change would be fine.

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #150 (e92d9be) into main (948b879) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #150   +/-   ##
=======================================
  Coverage   57.86%   57.86%           
=======================================
  Files          34       34           
  Lines        2041     2041           
=======================================
  Hits         1181     1181           
  Misses        860      860           
Impacted Files Coverage Δ
src/types/chrono.rs 80.43% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

2 participants