Skip to content

Conversation

SebastianKeller
Copy link

This is a fix for #10773
Now the progress always starts with 1. The counter is updated last, after every test related message has been printed.

Sebastian Keller added 2 commits February 11, 2022 00:57
Previously the progress displayed the first item as [0/x]. This was
misleading when x is the number of items. The first item should be
displayed as [1/x]
@Vexu Vexu merged commit f22443b into ziglang:master Feb 13, 2022
@SebastianKeller SebastianKeller deleted the fix_test_counter branch February 13, 2022 15:16
Vexu pushed a commit that referenced this pull request Feb 14, 2022
In #10859 I moved the `test_node.end()` call after everything else has
been logged. Now the `test_fn.name` is printed by `Progress` itself,
making the additional log obsolete.
kubkon pushed a commit that referenced this pull request Feb 16, 2022
Previously the progress displayed the first item as [0/x]. This was
misleading when x is the number of items. The first item should be
displayed as [1/x]
kubkon pushed a commit that referenced this pull request Feb 16, 2022
In #10859 I moved the `test_node.end()` call after everything else has
been logged. Now the `test_fn.name` is printed by `Progress` itself,
making the additional log obsolete.
@Snektron
Copy link
Collaborator

Snektron commented Jun 18, 2022

This change introduces an inconsistency: When the last item being worked on is printed, the count will be one more than the total count, and so the line will briefly display something like Progress [66/65]. Normally this is easily missed, however, it appears in a scenario where the time between updates is relatively long, and so when the final completeOne() is called it determines an update is required, and prints the incorrect message.

I would propose to either revert the change on this line:

const current_item = completed_items + 1;

as the problem in the related issue seems to be fixed by the other parts of this change, or to replace the final value with some special indicator, like Progress [done].

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.

4 participants