Re: [lmi] On the morality of using const_cast in unit tests

[Vadim Zeitlin]

On Wed, 3 Aug 2016 16:18:16 +0000 Greg Chicares <address@hidden> wrote:

GC> Vadim--Is it a code smell to use const_cast in unit tests?

 In general I'd say that using const_cast<> in order to really modify an
object via it (and not just to pass a pointer to the legacy and not
const-correct API) is indeed suspicious.

GC> Motivating case: commit fe56ee89d923767307228921f9b3a11c5f6769e1,
GC> abbreviated and rearranged for clarity here:
GC> 
GC>      database_entity const scalar(DB_PremTaxLoad, 0.0000);
GC> -    DBDictionary::instance().datum("PremTaxLoad") = scalar;
GC> +    DBDictionary& dictionary = const_cast<DBDictionary&>(*db.db_);
GC> +    dictionary.datum("PremTaxLoad") = scalar;
GC> 
GC> The unit test sets up this "what-if" circumstance in order to validate
GC> class premium_tax's operation.

 I think the way I'd do this would be to provide a way to test such what-if
scenario in product_database itself by allowing to replace its "db_"
member. Of course, these methods would need to be private (if we keep
premium_tax_test as a friend of this class) or be explicitly marked as 
being for testing only. But if we had them, then, instead of using a const
case, we could just make a copy of the original DBDictionary and then
create a product_database representing it:

        DBDictionary dictionary = db.get_db_dictionary();
        dictionary.datum("PremTaxLoad") = scalar;
        product_database dbForTest
                (dictionary
                ,... all the usual parameters or, maybe, yare_input
                );

I think this is preferable not only because it avoids the const_cast but
also because it avoids temporary modification to the original "db" object
and hence we don't have to worry about undoing it later. IMHO it's also
more clear.

GC> The original (prior to this month) cache held only one such container,
GC> and was non-const. The new caching implementation holds any number of
GC> such containers, and its API provides only const access (i.e., through
GC> a shared_ptr<T const>). This const API seemed like a good idea because
GC> production code should never change any cached database; but unit tests
GC> do have good reason to change it.

 FWIW I definitely agree that it makes sense for the API to be const.

GC> It might be better to use shared_ptr<T> instead, both in the caching
GC> templates, and in class product_database (which holds it as a private
GC> member and dereferences it only in a single const function anyway).
GC> Then friendship lets unit tests access the private (non-const) member
GC> without const_cast. What's your opinion?

 Originally I thought it would be a pity to lose constness guarantee
inside product_database code but after seeing that it's indeed only used in
a single place, this doesn't look like a big deal any more, so I agree that
it would be a slightly better alternative to the current code. But I'd
still prefer to keep the constness and just provide a way to inject a
modified DBDictionary into product_database for testing purposes as
described above.

 Regards,
VZ