Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

drobnikj
Copy link
Member

I just changed the parsing date as, the worker uses JS Date.toISOString() function to print the date into env vars.

@github-actions github-actions bot added this to the 55th sprint - Platform team milestone Jan 20, 2023
@github-actions github-actions bot added the t-core-services Issues with this label are in the ownership of the core services team. label Jan 20, 2023
Copy link
Member

@fnesveda fnesveda left a 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).

@@ -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)
Copy link
Member

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'

Copy link
Member

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

Copy link
Member Author

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"}

Comment on lines 34 to 36
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]}')
Copy link
Member

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.

Suggested change
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]}')

Copy link
Member Author

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.

Copy link
Member

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 Enums work in Python:

  • ApifyEnvVars is an Enum
  • ApifyEnvVars.MEMORY_MBYTES is an Enum member (not the value directly)
  • ApifyEnvVars.MEMORY_MBYTES.name is the Enum member name ('MEMORY_MBYTES')
  • ApifyEnvVars.MEMORY_MBYTES.value is the Enum member value ('APIFY_MEMORY_MBYTES')
  • INTEGER_ENV_VARS is a List of ApifyEnvVars 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)

@drobnikj drobnikj requested a review from fnesveda January 24, 2023 09:09
@drobnikj drobnikj merged commit cc45afb into master Jan 25, 2023
@drobnikj drobnikj deleted the feat/unit_test_env_helpers branch January 25, 2023 07:38
@fnesveda fnesveda added the validated Issues that are resolved and their solutions fulfill the acceptance criteria. label Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t-core-services Issues with this label are in the ownership of the core services team. validated Issues that are resolved and their solutions fulfill the acceptance criteria.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.