Page MenuHomeFreeBSD

Mk/Uses/kde.mk: Enable testing conditionally
ClosedPublic

Authored by jhale on Jan 21 2021, 5:06 PM.

Details

Summary
  • Allow autotests if TEST_TARGET is enabled. I would like to keep the TEST option for finance/libalkimia (update pending), but it is not possible if testing is globally disabled.
Test Plan

I've been dogfooding this patch for several months and haven't seen any adverse behavior.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhale requested review of this revision.Jan 21 2021, 5:06 PM

Moin moin

As someone who got his system wiped twice (before understanding why) by KDE tests that assume that tests are not run as root (and rm -r /etc should fail, ..., fun stuff), I'm slightly opposed to enable tests by default. That's also why it was explicitly turned off here.

But as far as I can see, you only enable them when explicitly setup in the port to do so, right?

Could you commit the cleanup-changes separately, please, nd then refresh the review?

mfg Tobias

Moin moin

As someone who got his system wiped twice (before understanding why) by KDE tests that assume that tests are not run as root (and rm -r /etc should fail, ..., fun stuff), I'm slightly opposed to enable tests by default. That's also why it was explicitly turned off here.

But as far as I can see, you only enable them when explicitly setup in the port to do so, right?

Genau (und tut mir leid)! The autotests would only be run if TEST_TARGET is defined, otherwise they are disabled.

Could you commit the cleanup-changes separately, please, nd then refresh the review?

I can do that.

Could you commit the cleanup-changes separately, please, nd then refresh the review?

I can do that.

Macro makeitso:

This revision was not accepted when it landed; it landed in state Needs Review.Jan 21 2021, 9:14 PM
This revision was automatically updated to reflect the committed changes.

Minor clean-up changes committed. Reopen for review of testing changes.

Update after minor clean-up commit.

I wonder whether it would be better to use a kde_ARGS?

like for example

USES=  kde:5,ctest

and do something like

.     if ${kde_ARGS:Mctest}
CMAKE_ARGS+=	-DBUILD_TESTING:BOOL=true
.    else 
CMAKE_ARGS+=	-DBUILD_TESTING:BOOL=false
.   endif

I wonder whether it would be better to use a kde_ARGS?

like for example

USES=  kde:5,ctest

and do something like

.     if ${kde_ARGS:Mctest}
CMAKE_ARGS+=	-DBUILD_TESTING:BOOL=true
.    else 
CMAKE_ARGS+=	-DBUILD_TESTING:BOOL=false
.   endif

Hmm...IMHO, that seems to be less intuitive. I think it is enough to deliberately define TEST_TARGET in an individual port Makefile to enable the autotests. It is opt-in.

Sure, you're probably right :)

This revision was not accepted when it landed; it landed in state Needs Review.Jan 21 2021, 10:21 PM
This revision was automatically updated to reflect the committed changes.