search

AppImageCommunity / libappimage

Implements functionality for dealing with AppImage files
https://appimage.org
Other
46 stars 29 forks source link

Properly notify desktop entry parsing errors #115

Closed azubieta closed 5 years ago

azubieta commented 5 years ago

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 commented 5 years ago

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

TheAssassin commented 5 years ago

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

azubieta commented 5 years ago

Too late, already added the mock AppImage

TheAssassin commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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...