Fix rowversion null behaviour

[Wraith2]

Fixes #255

Changed RowVersion/Timestamp null behaviour for SqlDataReader so that it returns a DBNull value instead of an empty byte[]. An AppContext switch has been added to allow reverting to the previous (broken but kept that way for compatibility) behaviour, it's called Switch.Microsoft.Data.SqlClient.LegacyRowVersionNullBehaviour and is cached on first check for the entire process.

N.B. this is a breaking behavioural change.

/cc @dbrownems @bdebaere @mburbea

[David-Engel]

LGTM

[ErikEJ]

No tests??

[Wraith2]

No tests??

The appcontext switch is process global so I couldn't deterministically make sure that it turned on and off for the right tests. It's also a trivial change tested locally so it didn't seem worth doing lots of private reflection.

[roji]

The appcontext switch is process global so I couldn't deterministically make sure that it turned on and off for the right tests. It's also a trivial change tested locally so it didn't seem worth doing lots of private reflection.

I think it should be possible to turn on an appcontext switch as part of the test (i.e. AppContext.SetSwitch), and back off at the end of it; take care to also disable parallelization for that test to avoid affecting other tests running in parallel.

(no idea on whether testing is important for this particular case etc.)

[ErikEJ]

But it should be possible to add a test to confirm the new changed default behaviour? Or do I misunderstand?

[Wraith2]

If I can find how to prevent the tests running in parallel as @roji says is possible then yes it can be tested.
I just hoped I could get away without having to write one for something this trivial, I really hate writing tests.

[ErikEJ]

It should be possible to write a test confirming the new behaviour without changing any AppContext switches etc...

[Wraith2]

Positive test added.

[cheenamalhotra]

If I can find how to prevent the tests running in parallel as @roji says is possible then yes it can be tested.

@Wraith2
Our tests don't run in parallel in single process.
We run multiple processes for different configurations only.

We also change AppContext switch in other tests to validate behavior before and after, would recommend the same here.

[Wraith2]

Should I take that to mean that you wan me to add a negative test?

[Wraith2]

Negative test added and positive test reworked to use the same way of ensuring that the test has the correct app context value set. I've used private reflection to set the internal field value and then restore it after each test, without doing this the per-process cache will prevent both tests succeeding in the same process. A simple lock prevents them running concurrently which is safe because they're very fast tests.