Review Request 128533: Create a test that validates projects' appstream information

[Harald Sitter]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128533/#review98085
-----------------------------------------------------------


Fix it, then Ship it!




I always get dizzy when I see code that is meant to test something but is itself not tested :P
I am fine with it either way. Some style and functional complaints though.


kde-modules/KDECMakeSettings.cmake (line 133)
<https://git.reviewboard.kde.org/r/128533/#comment66054>

    Shouldn't there be visual hint of it being found/notfound? Otherwise how would one know that one might want to install it for testing.



kde-modules/KDECMakeSettings.cmake (line 135)
<https://git.reviewboard.kde.org/r/128533/#comment66055>

    excess space before opening bracket



kde-modules/KDECMakeSettings.cmake (line 137)
<https://git.reviewboard.kde.org/r/128533/#comment66056>

    excess space before opening bracket



kde-modules/appstreamtest.cmake (line 13)
<https://git.reviewboard.kde.org/r/128533/#comment66057>

    excess space before opening bracket



kde-modules/appstreamtest.cmake (line 20)
<https://git.reviewboard.kde.org/r/128533/#comment66060>

    This will ignore the return value of the validation. Making the ctest summary always claim success, which is weird and meh.
    
    * make test won't ever output anything useful
    * ctest won't output anyting useful unless run with `-V` or somesuch magic
    
    If you actually pick up the result of the validation and then `message(STATUS` on ==0 or `message(FATAL_ERROR`, I think usefulness will go up 300% by actually claiming a validation fail when there was a fail :)


- Harald Sitter


On Aug. 3, 2016, 11:21 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128533/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2016, 11:21 p.m.)
> 
> 
> Review request for Build System, Extra Cmake Modules, KDE Frameworks, Matthias Klumpp, Scarlett Clark, and Harald Sitter.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> At the moment, we're validating it in build.kde.org, but I feel it will be easier for developers to test if we do so locally.
> This patch does it by seeing which `*.appdata.xml` files are being installed and validating them. This way we can keep it generic for all KDE projects.
> 
> 
> Diffs
> -----
> 
>   kde-modules/KDECMakeSettings.cmake dd37e7f 
>   kde-modules/appstreamtest.cmake PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/128533/diff/
> 
> 
> Testing
> -------
> 
> Tested on some projects, locally.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20160804/7069c0bd/attachment.html>