Remove unclear Advance() method from FakeTimeProvider

[egil]

The Advance() method requires the reader to know implicitly that it is 1ms that time is being advanced with (or read docs/use an editor that shows the info), and it is an overload that can be easily added via an extension method by teams that have the need. So since the goal is a generally applicable fake, I would recommend keeping the API surface clean and not including Advance() out of the box.

Closes #4007

Microsoft Reviewers: Open in CodeFlow

[geeknoid]

I don't agree with this change, I find the Advance method convenient. For many tests, all you need is for time to move forward and this overload just encourages consolidating around millisecond boundaries in tests, which is clean and simple.

We've gone through API reviews already for this type, so we'd want to review this again before nuking the API.

[rafal-mz]

I agree with the author of the PR. I did like the advance method, when we could set the advance amount in the constructor of FakeTimeProvider. I think 1 millisecond is too specific value to be universally useful, yet as author pointed out it forces to read the library code to understand what is happening. I don't think this overload hurts though, I would not use it myself (without setting its advance value at ctor which I liked a lot).

As a second thought, AdvanceMilisecond would be IMO better. But then question, why only milisecond, and not AdvanceSecond, AdvanceMinute and so on.

[egil]

For many tests, all you need is for time to move forward ...

That may be the case for tests in dotnet/extensions, but I have not had that experience when using (something like) this to test business logic yet, so I don't think this statement is universally correct.

With business logic, you often have rules like "after 5 minutes, the order status changes to ...".

... AdvanceMilisecond would be IMO better. But then question, why only milisecond, and not AdvanceSecond, AdvanceMinute and so on.

I don't mind these convenience methods, at least they make it clear what's going on, which helps with test readability.

[danmoseley]

We've gone through API reviews already for this type

Doh -- I did not think of API review process as it's a test only fake, but have not gotten used to the exciting world where we're shipping fakes 😺 I misled you @egil .

I'm not sure what our API review process is for this repo, but meantime, the commenting folks above effectively decide here. I have no opinion myself.

[egil]

We've gone through API reviews already for this type

Doh -- I did not think of API review process as it's a test only fake, but have not gotten used to the exciting world where we're shipping fakes 😺 I misled you @egil .

All good. I'm just throwing in my two cents here and trying to learn about your processes and thinking.

[joperezr]

Thanks for the contribution @egil. I think I agree with @rafal-mz as well in that for readability we should either remove this method, or make it more self-explanatory by renaming it appropriately. No super strong opinions there, but seems like removing it in favor of the use of the one with the parameter seems like the better option. @geeknoid Thoughts regarding the replies given above?