-
Notifications
You must be signed in to change notification settings - Fork 12
feat: add test for get_env and is_at_home #29
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some small suggestions (and one problem with the datetime parsing).
src/apify/_utils.py
Outdated
@@ -155,7 +155,7 @@ def _maybe_parse_bool(val: Optional[str]) -> bool: | ||
|
||
def _maybe_parse_datetime(val: str) -> Union[datetime, str]: | ||
try: | ||
return datetime.strptime(val, '%Y-%m-%dT%H:%M:%S.%fZ').replace(tzinfo=timezone.utc) | ||
return datetime.fromisoformat(val) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work, because the JS ISO format coming from the worker is different than the ISO format Python uses (no Z
suffix for UTC timezone) 🤦 So this has to be like it was.
>>> datetime.fromisoformat('2023-01-20T16:20:11.655Z')
ValueError: Invalid isoformat string: '2023-01-20T16:20:11.655Z'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's sync on monday and discuss whether we want to use formats with timezone or not. I have exclusively used datetime.utcnow()
which does not have timezone included
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss it mabe we shoule use dateunit as it looks can pase this string.
I'm afraid we can found issue in more places, I just test stringify JSON in JS and it looks same format:
JSON.stringify({'date': new Date('2023-01-20T16:20:11.655Z')})
{"date":"2023-01-20T16:20:11.655Z"}
int_get_env_var = print_get_env_var_attr(int_env_var) | ||
expected_get_env[int_get_env_var] = random.randint(1, 99999) | ||
monkeypatch.setenv(int_env_var, f'{expected_get_env[int_get_env_var]}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just use env_var.name.lower()
here, to get the name of the enum key, without having to do the replace
.
int_get_env_var = print_get_env_var_attr(int_env_var) | |
expected_get_env[int_get_env_var] = random.randint(1, 99999) | |
monkeypatch.setenv(int_env_var, f'{expected_get_env[int_get_env_var]}') | |
env_var_name = int_env_var.name.lower() | |
expected_get_env[env_var_name] = random.randint(1, 99999) | |
monkeypatch.setenv(int_env_var, f'{expected_get_env[env_var_name]}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it looks it works. but I have not sure why. Can you explain a little bit, looks that INTEGER_ENV_VARS is List and then int_env_var is just an item of the list, a dummy string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all because of how Enum
s work in Python:
ApifyEnvVars
is anEnum
ApifyEnvVars.MEMORY_MBYTES
is anEnum
member (not the value directly)ApifyEnvVars.MEMORY_MBYTES.name
is theEnum
member name ('MEMORY_MBYTES'
)ApifyEnvVars.MEMORY_MBYTES.value
is theEnum
member value ('APIFY_MEMORY_MBYTES'
)INTEGER_ENV_VARS
is aList
ofApifyEnvVars
members, not their values directly
It works a bit differently than in TypeScript, where accessing the enum member gives you its value directly, not the whole member object.
The monkeypatch.setenv(int_env_var, ...)
should really be monkeypatch.setenv(int_env_var.value)
, to be completely correct. It works right now because internally the os.environ[int_env_var]
will do int_env_var.encode(...)
or int_env_var.upper()
before setting the env var (depending on whether you're on UNIX or Windows), which returns the enum member value, and it works. (See https://github.com/python/cpython/blob/main/Lib/os.py#L745-L772)
I just changed the parsing date as, the worker uses JS
Date.toISOString()
function to print the date into env vars.