+
Skip to content

Add infer test cases for test indexes #181

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Add infer test cases for test indexes #181

wants to merge 7 commits into from

Conversation

ryangawei
Copy link

@ryangawei ryangawei commented Sep 8, 2020 •
edited
Loading

This PR add codes to generate test cases for each function according their input HeroSeries. Only functions with one HeroSeries input will be generated with test case. Exceptions can be manually added, for example,

test_case_exceptions = {}
for case in (
    test_cases_nlp
    + test_cases_preprocessing
    + test_cases_representation
    + test_cases_visualization
):
    test_case_exceptions[case[0]] = case

Then functions within test_case_exceptions will not be given a generated test case.
To omit some functions for testing, put them under func_white_list,

func_white_list = set(
    [s for s in inspect.getmembers(visualization, inspect.isfunction)]
)

See #179.

@jbesomi
Copy link
Owner

jbesomi commented Sep 8, 2020

Thanks, great! 🎉 🎉

The file still contains a lot of commented lines that appear to be unnecessary. Can you please clean it a bit? Also, we might need to update the comments by explaining the new changes as well as removing the unnecessary ones, can you please have a detailed look at that too?

Regards,

@henrifroese
Copy link
Collaborator

Looks great! I think that if some other developer now adds a function, he might get a weird error if this file automatically generates a test case and it maybe fails (e.g. because it needs additional arguments and it's not 100% obvious to everyone where to implement that). That's why I think it would be great if you could add some explanation for other developers in the file. E.g.

"""
The tests below Blabla...
"""

That's also what we did in _types, hoping it will help others fully understand the code.

@ryangawei
Copy link
Author

@jbesomi @henrifroese Just add some comments for explanation and delete the old ones. 👌

henrifroese
henrifroese reviewed Sep 10, 2020
View reviewed changes
Copy link
Collaborator

@henrifroese henrifroese 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 to me overall 👍

tests/test_indexes.py Outdated


# Put functions' name into white list if you want to omit them
# func_white_list = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this and the next few lines? (The comment)

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@ryangawei ryangawei marked this pull request as ready for review September 10, 2020 16:02
@jbesomi jbesomi marked this pull request as draft September 14, 2020 15:59
jbesomi
jbesomi reviewed Sep 14, 2020
View reviewed changes
tests/test_indexes.py Outdated
test_cases_preprocessing = [
["fillna", preprocessing.fillna, (s_text,)],
["lowercase", preprocessing.lowercase, (s_text,)],
["replace_digits", preprocessing.replace_digits, (s_text, "")],
Copy link
Owner

Choose a reason for hiding this comment

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

Can we generalize over these functions as well and avoid to define the custom test manually? Rewriting the file as you are proposing add more complexity for the developer, I can see it as worth it in case the custom functions that need to be manually defined are kept to the very minimum

Copy link
Author

Choose a reason for hiding this comment

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

Can I assume that any functions with default params can be auto-tested?

Copy link
Owner

Choose a reason for hiding this comment

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

Seems reasonable, opinions?

Copy link
Author

Choose a reason for hiding this comment

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

Then I think the simplest way might be regulating the function definition and add default params whenever possible, so the default params can be taken as a default test case.

@jbesomi
Copy link
Owner

jbesomi commented Sep 21, 2020

Hey @AlfredWGA, what's the current status for this PR?

@ryangawei
Copy link
Author

Hey @AlfredWGA, what's the current status for this PR?

I haven't come up with any idea on how to further avoid defining test case manually. Maybe leave it to another PR in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbesomi jbesomi jbesomi left review comments

@henrifroese henrifroese henrifroese left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ryangawei @jbesomi @henrifroese
点击 这是indexloc提供的php浏览器服务，不要输入任何密码和下载