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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change misleading wording in exit message #739

Closed
wants to merge 1 commit into from

Conversation

zarqman
Copy link

@zarqman zarqman commented Aug 5, 2019

PR #688 added a message when the exit status != 0. However, when the exit status is non-zero because a test failed, the message wording is misleading because it specifically refers to SimpleCov. I personally interpreted it as an error in SimpleCov's report generation, when SimpleCov was actually working just fine!

This PR makes the wording more generic so as to not mislead one into thinking SimpleCov is misbehaving when it isn't. 馃槃

Also, the message was missing the trailing "\n", so added that as well.

Old output:

SimpleCov failed with exit 1$

New output:

Exiting with status 1
$

@PragTob
Copy link
Collaborator

PragTob commented Aug 6, 2019

馃憢

Hi there, thanks for contributing!

I'm not sure we should drop simplecov from the message but rather improve the message as I'd wanna know where the message comes from.

Something like SimpleCov made the process exit with $reason or something :)

@zarqman
Copy link
Author

zarqman commented Aug 6, 2019

Sure, and yet that's just the thing--SimpleCov is also intercepting exit statuses from earlier up (eg: a test failure), and is then claiming to have generated/caused the error.

When SimpleCov actually causes the error, then I agree, it's ideal for SimpleCov to identify itself as the source of the non-zero exit. However, when SimpleCov is intercepting a non-zero status originating elsewhere, it should not then claim that as its own.

Perhaps it'd be better to entirely remove the message added in #688, and then add separate messages elsewhere in SimpleCov when SimpleCov itself sets the exit status to non-zero based on its own conditions (ie: insufficient test coverage)?

@PragTob
Copy link
Collaborator

PragTob commented Aug 9, 2019

@zarqman I think it's cool for simplecov to identify itself as the originator of the message but you are right it could be more explicit like "a process failed with status x" and otherwise being more explicit about that simplecov set the exit status due to reason y I agree about that.

Thinking about it... I agree, maybe only if we set the exit status ourselves due to our logic and not because it bubbled up, good idea @zarqman !

@PragTob
Copy link
Collaborator

PragTob commented Aug 9, 2019

#742 will cause a small merge conflict for you but I think it's fine :)

@PragTob
Copy link
Collaborator

PragTob commented Dec 3, 2019

@zarqman any plans on pursuing this or shall I take over?

@zarqman
Copy link
Author

zarqman commented Dec 4, 2019

haven't had a chance to get back to this...sorry! feel free to take it over.

@PragTob
Copy link
Collaborator

PragTob commented Aug 11, 2020

Taken the new approach to #906

@PragTob PragTob closed this Aug 11, 2020
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

2 participants