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

Tests for core and cli modules #149

Merged
merged 80 commits into from
Nov 5, 2021
Merged

Tests for core and cli modules #149

merged 80 commits into from
Nov 5, 2021

Conversation

alex-zenml
Copy link
Contributor

Pre-requisites

Please ensure you have done the following:

  • I have read the CONTRIBUTING.md document.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have added tests to cover my changes.

Types of changes

  • Testing (non-breaking tests added)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Describe changes

Add tests for core and cli modules
Some tiny typos and bugs found in src fixed alongside.

Copy link
Contributor

@schustmi schustmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice, just got a few minor comments

tests/cli/test_base.py Show resolved Hide resolved
tests/cli/test_config.py Outdated Show resolved Hide resolved
tests/core/test_utils.py Show resolved Hide resolved
tests/cli/test_example.py Show resolved Hide resolved
current_saved_global_examples_repo.git.reset("--hard", "0.5.0")
runner.invoke(pull, ["-f", "-v", "0.5.1"])
result = runner.invoke(list)
assert "airflow_local" in result.output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this call raise an error somehow if the version does not exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schustmi I didn't understand this. Do you mean that we should add another test to catch this condition (i.e. if the version doesn't exist) or that the currently-written test is doing something unexpected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure actually how this should be handled, maybe a warning message that the requested version does not exists and that we fallback to the latest one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alex-zenml wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a warning message.

tests/core/test_local_service.py Show resolved Hide resolved
tests/core/test_local_service.py Outdated Show resolved Hide resolved
tests/core/test_repo.py Show resolved Hide resolved
tests/core/test_repo.py Show resolved Hide resolved
Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an incomplete review from a week ago or something. Its just two comments that I thought might still be relevant.

readme_content = git_examples_handler.get_example_readme(example_dir)
click.echo(readme_content)
except FileNotFoundError:
error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a bit weird because maybe an example is there but doesnt have a README and it would say the example is not present

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now.

assert "local_metadata_store" in result.output


# test metadata register command actually registers a new metadata store
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these all TODO's?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was leftover from an initial session thinking through the code. Removed now.

@alex-zenml
Copy link
Contributor Author

@schustmi back over to you now. I addressed all your comments. Thanks for the review. It's much better now thanks to them!

Copy link
Contributor

@htahir1 htahir1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Just comments to remove some hanging comments otherwise LGTM!

@@ -181,3 +184,14 @@ def parse_unknown_options(args: List[str]) -> Dict[str, Any]:
assert len(p_args) == len(r_args), "Replicated arguments!"

return r_args


# def markdown_to_console(markdown: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we take these out?

@@ -21,6 +21,9 @@

from zenml.core.base_component import BaseComponent

# from rich.console import Console
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we take these out?

f"Example {example_name} is not one of the available options."
f"\nTo list all available examples, type: `zenml example list`"
)
if path_utils.file_exists(example_dir) and path_utils.is_dir(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@alex-zenml
Copy link
Contributor Author

@htahir1 done!

I'll certainly return to the markdown parser feature. I added it and it works, but I couldn't figure out a good way to test it like we are now testing the non-parsed .md text. I'll add it to the backlog. So much more pleasant an experience compared to what we have currently.

@htahir1
Copy link
Contributor

htahir1 commented Nov 4, 2021

Cool! @schustmi can also take a look and merge then if he thinks everything is fixed :-)

@alex-zenml
Copy link
Contributor Author

@schustmi LMK if this is what you were thinking about with some kind of warning/notification of the redownload.

@schustmi
Copy link
Contributor

schustmi commented Nov 5, 2021

@alex-zenml Yes looks good!

@schustmi schustmi merged commit 4987db5 into main Nov 5, 2021
@schustmi schustmi deleted the alex/ENG-44-tests-core-cli branch November 5, 2021 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal To filter out internal PRs and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants