-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add perf. run duration estimation to PR comment #1654
Conversation
|
I have wished before for a link to the status page in the bot's comment to be able to see the queue, and since you're changing this code here, could you add that link as well please ? 🙏 |
63db906
to
0721201
Compare
|
Done, the queue text is now a link to the status page. |
0721201
to
964524f
Compare
The date recorded is the end of the collection, so we shouldn't add anything to it. It was implemented correctly for Postgres, but incorrectly for SQLite.
964524f
to
11e5728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to avoid publishing nonsense about instant collection if there's not a reasonable timestamp, but this seems ok for now.
11e5728
to
3de3d06
Compare
|
I added a min. estimated time of 1 hour, if data is missing or if it's too fast for some reason. |
Little QoL improvement that adds a rough estimate of how long it should take until a perf. run result is available. This information is added to the "Queued , future comparison URL..." comment on PRs.
It's not precise, since the duration of individual runs can vary, sometimes a perf. run can fail soon, and sometimes a PR run "jump ahead" another one, so that it takes longer. But I think that it's nice to know at least a (probable) lower bound of the waiting time, not everyone knows of the status page, and even if you know about it, this will show you its current state more quickly.
The "queued" comment is automatically hidden when the benchmark run finishes, so the outdated/stale "There are currently N other artifacts..." text won't be visible for a long time.