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

Merged
merged 10 commits into from
Nov 4, 2024
Merged

Report name #2947

merged 10 commits into from
Nov 4, 2024

Conversation

obriat
Copy link
Contributor

@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

Olivier Briat added 2 commits October 18, 2024 10:58
Signed-off-by: Olivier Briat <olivier.briat@kleegroup.com>
… doc about dev

Signed-off-by: Olivier Briat <olivier.briat@kleegroup.com>
@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
Contributor 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
Contributor 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
Contributor 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, ...)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea!

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

    time_parts = [
        (days, "day"),
        (hours, "hour"),
        (minutes, "minute"),
        (seconds, "second")
    ]

    parts = [f"{value} {label}{'s' if value != 1 else ''}" for value, label in time_parts if value > 0]

    return ', '.join(parts) if parts else "0 seconds"

Think something like this would do the trick (disclaimer: haven't tested it)

Copy link
Contributor Author

@obriat obriat Oct 29, 2024

Choose a reason for hiding this comment

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

Since I really want to keep the last "and" 😄 , here a working proposition:

def format_duration(start_time, end_time):
    seconds = end_time - start_time
    days = seconds // 86400
    hours = seconds // 3600
    minutes = (seconds % 3600) // 60
    seconds = seconds % 60

    time_parts = [(days, "day"), (hours, "hour"), (minutes, "minute"), (seconds, "second")]

    parts = [f"{value} {label}{'s' if value != 1 else ''}" for value, label in time_parts if value > 0]

    return " and ".join(filter(None, [", ".join(parts[:-1])] + parts[-1:])) if parts else "0 seconds"

0: 0 seconds
666: 11 minutes and 6 seconds
1332: 22 minutes and 12 seconds
1998: 33 minutes and 18 seconds
2664: 44 minutes and 24 seconds
3330: 55 minutes and 30 seconds
3996: 1 hour, 6 minutes and 36 seconds
4662: 1 hour, 17 minutes and 42 seconds
5328: 1 hour, 28 minutes and 48 seconds
5994: 1 hour, 39 minutes and 54 seconds
6660: 1 hour and 51 minutes
7326: 2 hours, 2 minutes and 6 seconds
7992: 2 hours, 13 minutes and 12 seconds

One thing I don't get, is how to write a test the right way:

At the moment, swarmReportMock.duration is a string that is checked in HtmlReport.test.psx, but it seems like a static markup check not a real verification of the the computed difference between swarmReportMock.endTime and swarmReportMock.startTime .

Should this check be done here (and how) or should the math check be done in another test case (pure python?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can do something like so:

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

    time_parts = [
        (days, "day"),
        (hours, "hour"),
        (minutes, "minute"),
        (seconds, "second"),
    ]

    parts = [
        f"{value} {label}{'s' if value != 1 else ''}"
        for value, label in time_parts
        if value > 0
    ]
    parts[-1] = "and " + parts[-1]

    return ", ".join(parts) if parts else "0 seconds"

To avoid iterating over the list multiple times and help keep things readable?

If you have time, maybe a couple of basic unit tests for the function would also be good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It returns an error 😞 and I didn't find the way to fix it, so I push my version (which I tested with a loop).
I'll be happy to have some tips about unit tests, I'll add them if I had some more spare times

Copy link
Collaborator

@andrewbaldwin44 andrewbaldwin44 Oct 30, 2024

Choose a reason for hiding this comment

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

Ah I think the error probably happens in the "0 seconds" case right? In which case you could do:

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

    time_parts = [
        (days, "day"),
        (hours, "hour"),
        (minutes, "minute"),
        (seconds, "second"),
    ]

    parts = [
        f"{value} {label}{'s' if value != 1 else ''}"
        for value, label in time_parts
        if value > 0
    ]

    if parts:
        if len(parts) > 1:
            parts[-1] = "and " + parts[-1]
        return ", ".join(parts)
    else:
        return "0 seconds"

Some test cases could look like:

from datetime import datetime, timedelta
from .testcases import LocustTestCase
from .util.date import format_duration

class TestFormatDuration(LocustTestCase):
    def setUp(self):
        super().setUp()

    def test_zero_seconds(self):
        start_end_time = datetime(2023, 10, 1, 12, 0, 0)
        self.assertEqual(format_duration(start_end_time, start_end_time), "0 seconds")

    def test_seconds_only(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 1, 12, 0, 30)
        self.assertEqual(format_duration(start_time, end_time), "30 seconds")

    def test_minutes_only(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 1, 12, 45, 0)
        self.assertEqual(format_duration(start_time, end_time), "45 minutes")

    def test_hours_only(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 1, 15, 0, 0)
        self.assertEqual(format_duration(start_time, end_time), "3 hours")

    def test_days_only(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 4, 12, 0, 0)
        self.assertEqual(format_duration(start_time, end_time), "3 days")

    def test_days_hours_minutes_seconds(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 3, 15, 45, 30)
        self.assertEqual(format_duration(start_time, end_time), "2 days, 3 hours, 45 minutes, and 30 seconds")

    def test_singular_units(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 2, 13, 1, 1)
        self.assertEqual(format_duration(start_time, end_time), "1 day, 1 hour, 1 minute, and 1 second")

    def test_no_days(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 1, 15, 30, 45)
        self.assertEqual(format_duration(start_time, end_time), "3 hours, 30 minutes, and 45 seconds")

    def test_no_hours(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 2, 12, 30, 45)
        self.assertEqual(format_duration(start_time, end_time), "1 day, 30 minutes, and 45 seconds")

    def test_no_hours_no_minutes(self):
        start_time = datetime(2023, 10, 1, 12, 0, 0)
        end_time = datetime(2023, 10, 2, 12, 0, 45)
        self.assertEqual(format_duration(start_time, end_time), "1 day, and 45 seconds")

    def test_will_not_format_years(self):
        start_time = datetime(2023, 1, 1, 0, 0, 0)
        end_time = start_time + timedelta(days=400)
        self.assertEqual(format_duration(start_time, end_time), "400 days")

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 add tests, could not find a way to use parametrize with class / self 😞

@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 👍

@cyberw
Copy link
Collaborator

cyberw commented Nov 3, 2024

I tested --html just for fun :) It doesn't seem to have the duration?

image

Once you've fixed it, maybe add a search for ' seconds' here to ensure it works

self.assertIn('"show_download_link": false', html_report_content)

@cyberw
Copy link
Collaborator

cyberw commented Nov 3, 2024

Btw, resolve the conflicts in launch.json whatever way you like, dont worry about my changes :)

@obriat
Copy link
Contributor Author

obriat commented Nov 3, 2024

I’ll fix the conflict, a bit strange about the —html I’m pretty sure I tested it :(
Is there a way to make a automated test about it?

@obriat
Copy link
Contributor Author

obriat commented Nov 3, 2024

I tested --html just for fun :) It doesn't seem to have the duration?

"It works on my PC":

git branch --show-current
report-name
poetry run locust -f examples/basic.py  -u 1 -H http://192.168.64.100:8089 --headless -u 1 -t 30s --html ~/headless-locust-report.html
grep -Eo '"duration": "[^"]*"'  ~/headless-locust-report.html
"duration": "29.067671298980713 seconds"

The rendered html is also correct "During: 03/11/2024 23:01:02 - 03/11/2024 23:01:31 (29.067671298980713 seconds)"

(Yes, I know, I got a round problem 😄 )

@cyberw
Copy link
Collaborator

cyberw commented Nov 4, 2024

Yea sorry, missing the seconds was my bad, I must have messed up rebuilding the UI.

Test looks good, but we dont have pytest as a dependency so looks like it is failing. I think we'll be going in the pytest direction in not too long a while, so feel free to add it to the dev dependencies (something like poetry add --group dev pytest)

@obriat
Copy link
Contributor Author

obriat commented Nov 4, 2024

Done, pyproject.toml changes are readable, but I don't know about the poetry.lock ones.

@cyberw
Copy link
Collaborator

cyberw commented Nov 4, 2024

Sorry, it was the wrong group, it was supposed to be added to "test", not "dev". I'll fix it in master. Thanks!

@cyberw cyberw merged commit 903122a into locustio:master Nov 4, 2024
12 of 17 checks passed
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