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

Fix handling of totals in ProgressReporter #114

Merged
merged 1 commit into from
Jun 26, 2014
Merged

Fix handling of totals in ProgressReporter #114

merged 1 commit into from
Jun 26, 2014

Conversation

ryankon
Copy link
Contributor

@ryankon ryankon commented Jun 25, 2014

Here are my changes per pull request #113 and issue #112. Instead of repeatedly setting the total every time a test is run, we can set it once when we start the progress bar, presumably because the number of tests should not be changing during a test run.

@@ -35,6 +35,7 @@ def start
puts 'Started'
puts
@progress.start
@progress.total = total_count
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps it is better to set total before starting the progress?
Now this order also works but looks unnatural and at some point progress may want to use total in start.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also do we still have to provide total in progress' constructor if we know that it is always nil there?

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 originally tried to do both those things. I agree that it is a bit unnatural this way, but setting total prior to starting actually breaks the output for the empty test class.

image

Not setting total to nil in the constructor works too, but it defaults the total to 100, which you see briefly prior to starting, and I preferred the current appearance of briefly seeing the uninitialized progress bar instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for explanation.

os97673 added a commit that referenced this pull request Jun 26, 2014
Fix handling of totals in ProgressReporter
@os97673 os97673 merged commit 04c4326 into minitest-reporters:master Jun 26, 2014
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