Made datetime attribute insert and match time-zone invariant #301
Conversation
…alue parsing for datetime fields in AttributeMatcher and ValueMatcher
…zone step for scenarios in BDD, fixed AttributeMatcher and ValueMatcher parsing
shiladitya-mukherjee
requested review from
alexjpwalker and
flyingsilverfin
as code owners
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
…concept_proto builder
…concept_proto builder
shiladitya-mukherjee
changed the title
shiladitya-mukherjee
changed the title
| def step_impl(context: Context, time_zone_label: str): | ||
| os.environ["TZ"] = time_zone_label | ||
| time.tzset() | ||
|
|
There was a problem hiding this comment.
Should we reset it after our test is done, so we don't affect other tests?
There was a problem hiding this comment.
We have to do it in typedb-behaviour. Can we assume that the server time is always UTC?
There was a problem hiding this comment.
No, we can't assume that.
We could store the time zone and restore it using @Before/@after methods
https://cucumber.io/docs/cucumber/api/?lang=java
There was a problem hiding this comment.
Updating my comment with what Joshua said:
We should try to put it as an explicit step in the background.
| @@ -60,9 +60,13 @@ def parseWords(text): | |||
| @parse.with_pattern(r"\d\d\d\d-\d\d-\d\d(?: \d\d:\d\d:\d\d)?") | |||
| def parse_datetime(text: str) -> datetime: | |||
| try: | |||
| return datetime.strptime(text, "%Y-%m-%d %H:%M:%S") | |||
| return datetime.strptime(text, "%Y-%m-%dT%H:%M:%S") | |||
There was a problem hiding this comment.
Note: this won't ever run right? Because the regex above doesn't accept a T ever. Let's discuss why you made this change tomorrow
There was a problem hiding this comment.
This function was called also from inside the Python client, on top of it being called by Cucumber.
shiladitya-mukherjee
force-pushed
the
date-time-fix
branch
from
b8b1fe6 to
0735e4c
Compare
| remote = "https://github.com/vaticle/typedb-behaviour", | ||
| commit = "767bf98fef7383addf42a1ae6e97a44874bb4f0b" # sync-marker: do not remove this comment, this is used for sync-dependencies by @vaticle_typedb_behaviour | ||
| remote = "https://github.com/shiladitya-mukherjee/typedb-behaviour", | ||
| commit = "ac3006755c13e40bb1404f82ab9e65d4255e3cce" # sync-marker: do not remove this comment, this is used for sync-dependencies by @vaticle_typedb_behaviour |
There was a problem hiding this comment.
Since typedb-behaviour is now merged, You can now bump this to the new vaticle commit.
What is the goal of this PR?
Enables the BDD tests to check timezone-invariance of inserting and reading datetime attributes.
What are the changes implemented in this PR?
fromtimestampfunction. This function is environment timezone-sensitive. The correct function to use isutcfromtimestampwhich converts the milliseconds since epoch to UTC datetime object. This conforms to the serialization process: we convert datetime object to milliseconds since epoch assuming the datetime is in UTC.time_zone_labelAttributeMatcherandValueMatcher.Fixes #298.