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

Properly notify desktop entry parsing errors #115

Merged
merged 2 commits into from
May 17, 2019

Conversation

azubieta
Copy link
Contributor

No description provided.

Copy link
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Missing tests that check for this exception to be thrown.

@azubieta
Copy link
Contributor Author

This exception is thrown by the XdgUtils lib and it's already checked there. Adding another check on libappimage will require us to add a yet another broken AppImage file to the repository.

@TheAssassin
Copy link
Member

You can also use a mock object, but the code in here, even if it's "just error handling", needs testing IMO.

@TheAssassin
Copy link
Member

You can also create another issue and postpone that discussion. Let's rather move on.

@azubieta
Copy link
Contributor Author

Too late, already added the mock AppImage

@TheAssassin
Copy link
Member

I was talking about a mock object, not a mock AppImage. The idea was to test that unit test code, so it should be possible to test this code against a mock object or simply a desktop entry object with broken code. That should be possible in a good software architecture. Adding more and more bloaty AppImages for testing is a bad idea.

@azubieta
Copy link
Contributor Author

I see, I thought you were talking about a whole AppImage. As I said XdgUtils already test the scenario where the desktop file is malformed. This code only renames the exception. We definitively add a redundant test on the DesktopEntry parsing.

@TheAssassin
Copy link
Member

TheAssassin commented May 17, 2019

You depict this wrong. There is 5-10 lines of code in this library that you say doesn't have to be tested. It does have to be tested. It's code in here. That's not redundant, it's checking that your "renaming" or whatever works. Doesn't matter if it interacts with another library. As said, there's an option of mocking said library and triggering the the exception block that way, no need to use that library.
For each such situation there should be a unit test to validate this behavior. But there are no unit tests at all in this project anyway...

@TheAssassin TheAssassin merged commit 55663d9 into master May 17, 2019
@TheAssassin TheAssassin deleted the improve_desktop_entry_parsing_message branch May 17, 2019 16:31
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