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

Change test outputs #185

Merged
merged 2 commits into from
Oct 27, 2022
Merged

Conversation

aurorarossi
Copy link
Member

As I wrote in issue #181, the test outputs were hiding the summary table.
With this pull request I hid the long outputs and I set verbose = true to have more readable information in the summary table.

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #185 (e32ea38) into master (7a2cc82) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #185   +/-   ##
=======================================
  Coverage   97.47%   97.47%           
=======================================
  Files         109      109           
  Lines        6349     6349           
=======================================
  Hits         6189     6189           
  Misses        160      160           

@aurorarossi
Copy link
Member Author

aurorarossi commented Oct 25, 2022

With this PR the output is the following:
image
I attach two screenshot of the current output (where there is the summary table but is difficult to find):

image

image

@aurorarossi
Copy link
Member Author

Another option removing verbose = true would be:
image

@gdalle gdalle merged commit 4fce15b into JuliaGraphs:master Oct 27, 2022
@simonschoelly
Copy link
Contributor

@gdalle I would have appreciated if that would have still been left open for discussion.

@simonschoelly
Copy link
Contributor

Ah, I realized that I only communicated on slack, so there was no trace of me reviewing

@aurorarossi
Copy link
Member Author

@simonschoelly issue #181 is still open so we can discuss there if you have other suggestion.

@gdalle
Copy link
Member

gdalle commented Oct 27, 2022

@gdalle I would have appreciated if that would have still been left open for discussion.

My bad @simonschoelly, I thought it was sufficiently minor to approve directly and I didn't think of any downsides to having more detailed test outputs. I didn't see the discussion on Slack though, I'll go there to check it out. Mea culpa!

@simonschoelly
Copy link
Contributor

@gdalle No sorry, this is totally my fault. I had completely forgotten that I never started doing the code review here on github and was still in the progress of trying to figure out in what way the test outputs were changing. Of course it should not be necessary to double check on slack.

@gdalle
Copy link
Member

gdalle commented Nov 15, 2022

Alright then! I haven't been very Slacking recently but it should get better soon :)

@aurorarossi aurorarossi deleted the clean_output_tests branch November 21, 2022 09:53
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.

None yet

3 participants