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

stats summary refactoring #2193

Merged
merged 12 commits into from
Sep 19, 2022
Merged

Conversation

SamPosh
Copy link
Contributor

@SamPosh SamPosh commented Sep 11, 2022

Added get_stats_summary and get_percentile_stats_summary . This will return the stats as string , So that the string can be used in other places. I am using this to record it in Reportportal.
currently print_stats and print_percentile_stats functions are directly record the result into console_logger. But these functions return the same results into string so that it can be used by other log handler

@cyberw
Copy link
Collaborator

cyberw commented Sep 13, 2022

Did you end up just duplicating the code and not using the new functions from anywhere else? This doesnt make sense.

And current parameter is never used?

Also, you need to fix flake8 & black formatting errors.

@SamPosh
Copy link
Contributor Author

SamPosh commented Sep 14, 2022

Did you end up just duplicating the code and not using the new functions from anywhere else? This doesnt make sense.

And current parameter is never used?

Also, you need to fix flake8 & black formatting errors.

print_stats_summary is directly printing to console_logger. So I refactored that function into get_stats_summary and called it from print_stats but the unit test is checking the length of string. Since it is single string the length is always 1. So it is failed. Due to that I duplicated the function. May be I will try to create it as list of string then call it from print_stats so that it won't be a duplicate.

For black/flake8 formatting, as of now I am doing this from vscode web editor which doesn't have formatting option. I am trying to setup local dev environment and try to format it.

@cyberw
Copy link
Collaborator

cyberw commented Sep 14, 2022

print_stats_summary is directly printing to console_logger. So I refactored that function into get_stats_summary and called it from print_stats but the unit test is checking the length of string. Since it is single string the length is always 1. So it is failed. Due to that I duplicated the function. May be I will try to create it as list of string then call it from print_stats so that it won't be a duplicate.

Not sure what you mean, but duplication is bad :)

For black/flake8 formatting, as of now I am doing this from vscode web editor which doesn't have formatting option. I am trying to setup local dev environment and try to format it.

Yea... tbh, that kind of sounds like you'll have a lot of work ahead of you to make this reach the quality required for merge. You might consider having this as a fork, or trying to implement it inside your own locustfile or support lib.

Copy link
Contributor Author

@SamPosh SamPosh left a comment

Choose a reason for hiding this comment

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

Duplication is removed

@SamPosh
Copy link
Contributor Author

SamPosh commented Sep 17, 2022

print_stats_summary is directly printing to console_logger. So I refactored that function into get_stats_summary and called it from print_stats but the unit test is checking the length of string. Since it is single string the length is always 1. So it is failed. Due to that I duplicated the function. May be I will try to create it as list of string then call it from print_stats so that it won't be a duplicate.

Not sure what you mean, but duplication is bad :)

For black/flake8 formatting, as of now I am doing this from vscode web editor which doesn't have formatting option. I am trying to setup local dev environment and try to format it.

Yea... tbh, that kind of sounds like you'll have a lot of work ahead of you to make this reach the quality required for merge. You might consider having this as a fork, or trying to implement it inside your own locustfile or support lib.

@cyberw I removed the duplication.

@cyberw
Copy link
Collaborator

cyberw commented Sep 18, 2022

Looks a lot better now. Just fix the formatting and typing issues and it might be ready.

@SamPosh
Copy link
Contributor Author

SamPosh commented Sep 19, 2022

Looks a lot better now. Just fix the formatting and typing issues and it might be ready.

I have fixed the type hint issue.

@SamPosh
Copy link
Contributor Author

SamPosh commented Sep 19, 2022

Looks a lot better now. Just fix the formatting and typing issues and it might be ready.

formatting also done. type hint is fixed as well

locust/stats.py Outdated Show resolved Hide resolved
locust/stats.py Outdated Show resolved Hide resolved
locust/stats.py Outdated Show resolved Hide resolved
@cyberw cyberw merged commit 76b3450 into locustio:master Sep 19, 2022
@cyberw
Copy link
Collaborator

cyberw commented Sep 19, 2022

👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants