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

[Greg Chicares]

On 2016-08-03 17:08, Vadim Zeitlin wrote:
> 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.

Okay, it's purged in commit a202f8754d3b9bf728078203e87c469190753ce4.

Somehow I had gotten the notion that using const_cast in unit tests was a
recommended practice. Using it here just didn't seem right, so I searched
the web, hoping to discover a convincing rationale, but instead found that
it's not generally recommended.

> 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
>               );

Of course, today there is no product_database ctor that does that, so this
would require new code; and then the code we'd be unit-testing wouldn't be
exactly the same as the production code.

Another approach would be to create custom product files inside the unit
test, with all of the oddities that today we impose by force. That's the
cleanest way of all.

But these more ambitious changes take work, and other tasks compete for
our attention. It's hard to resist the urge to reimplement the whole
database. See, e.g., commit 80101cad521dfe9110db3786ff148f26d45083b1,
which reveals the appalling truth that the database structure is really:
  nonstd::map<nonstd::triplet<int_key_type, string_key_type, mapped_type>
                              ^^ not const  ^^ not const

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

I do agree that class product_database should have a const API, so I've
added a const accessor.