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

Report name #2947

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Report name #2947

wants to merge 5 commits into from

Conversation

obriat
Copy link

@obriat obriat commented Oct 20, 2024

  • Add host, start time and scenario name to report filename (should be easier to find and sort)
  • Add human friendly duration in html report
  • Add some doc about dev and debugging

Close #2931

@obriat obriat changed the title Report name Report name (#2931) Oct 20, 2024
@obriat obriat changed the title Report name (#2931) Report name Oct 20, 2024
@cyberw
Copy link
Collaborator

cyberw commented Oct 21, 2024

Can you do this without adding the humanfriendly dependency? Seems a bit much to add an entire package just to format one date...

There's already some info about debugging in running-in-debugger.rst, can you maybe add your info there instead (and link to it from developing-locust.rst)

@andrewbaldwin44 Any opinions on the TS stuff?

@andrewbaldwin44
Copy link
Collaborator

@obriat Could you also check that this works with the --html flag?

@obriat
Copy link
Author

obriat commented Oct 21, 2024

Can you do this without adding the humanfriendly dependency? Seems a bit much to add an entire package just to format one date...

I'm not a Python expert, but does this package adds a overhead to the runners or just the report part?

There's already some info about debugging in running-in-debugger.rst, can you maybe add your info there instead (and link to it from developing-locust.rst)

I'll move my doc to this page

@JavierUhagon
Copy link
Contributor

JavierUhagon commented Oct 21, 2024

I'm not a Python expert, but does this package adds a overhead to the runners or just the report part?

Hey there! Not a mantainer, but, I don't think the issue is adding a overhead, it's having to add a full package (that hasn't been updated since 2020) just to format a date for a report, I'd say that adding your own implementation would be better :)

You can always check how humanfriendly does it!
https://github.com/xolox/python-humanfriendly/blob/6758ac61f906cd8528682003070a57febe4ad3cf/humanfriendly/__init__.py#L402

It really isn't hard nor long!

@obriat
Copy link
Author

obriat commented Oct 28, 2024

@obriat Could you also check that this works with the --html flag?
Hi,
I think that I response to every suggestions and fix failed tests.

About the --html flag, this option needs a filename as input (not mandatory but throw an exception on writing file). So what should we do ?

  • Keep the current behaviour?
  • Use the default generated filename if no input is given (but detects if the input is a dir and not a full filename+path)?

"""
# Common time units, used for formatting of time spans.
time_units = (
dict(divider=1e-9, singular="nanosecond", plural="nanoseconds", abbreviations=["ns"]),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry maybe it wasn't clear, but I think an argument for using our own solution, rather than the library, is because as you can probably see, the library handles a lot more cases than we probably need. In Locust's case, we probably never need nanoseconds, microseconds, weeks, or years. I think we could go with a simpler approach, for example:

def format_duration(start_time, end_time):
    time_diff = end_time - start_time

    days = time_diff.days
    seconds = time_diff.seconds
    hours = seconds // 3600
    minutes = (seconds % 3600) // 60
    seconds = seconds % 60

    return f"{days} days, {hours} hours, {minutes} minutes, {seconds} seconds"

(disclaimer: ChatGPT)

Copy link
Author

Choose a reason for hiding this comment

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

Should I keep some methods in order to keep it human friendly (plural, no output for 0 values, ...)?

@andrewbaldwin44
Copy link
Collaborator

andrewbaldwin44 commented Oct 28, 2024

Hey @obriat, for the --html <filename> flag, I just meant to double-check it's still working as expected, since it also uses the get_html_report function that was modified, but looks fine so no need to change anything 👍

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.

Downloading report should provide a meaningful human name
4 participants