Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the current time for NewFakeClock's initial value #55

Merged
merged 1 commit into from
Apr 1, 2023
Merged

Use the current time for NewFakeClock's initial value #55

merged 1 commit into from
Apr 1, 2023

Conversation

DPJacques
Copy link
Collaborator

@DPJacques DPJacques commented Feb 24, 2023

This prevents callers from building tests that rely on implementation details. Typically by expecting a time or string value computed from NewFakeClock().Now().

This philosophy of preventing users from relying on implementation internals is similar to Go's random map iteration behavior. For users who depend on the initial clock time, NewFakeClockAt() provides the behavior they need, and makes their expectations clear in tests.

This also ensures that future changes that might need to modify the default time for whatever reason won't cause mass breakage, as this author learned about the hard way.

@sagikazarmark
Copy link
Collaborator

This also ensures that future changes that might need to modify the default time for whatever reason won't cause mass breakage

Well, this change will. :)

Frankly, I agree with your reasoning, but I'm also worried about the potential mass new issue opening that it may cause.

One thing I would certainly do is expand in the function documentation a bit, telling people that they shouldn't rely on the time being deterministic when using NewFakeClock in their tests. I know it's somewhat redundant with mentioning randomness, but I feel like people need to be explicitly warned.

@DPJacques
Copy link
Collaborator Author

No changes made yet, pending discussion.

I'm also worried about the potential mass new issue opening that it may cause.

I would be too, until I realized the cat is already out of the bag on that one with #52.

I made this CL because I changed the default in that update, thinking .oO(Surely people wouldn't rely on the default time from NewFakeClock. That would be poor form, and NewFakeClockAt is like right there.).

Well, Hyrum's law had something to say about that. There were people in our code base depending on it. However, I went through the process of upgrading our code base to use NewFakeClockAt where people needed a specific time. I viewed it as a reasonable code clean up, although maybe it was premature.

All that is to say, which direction would you like to go with this?

  1. We can close this PR and add a warning to existing code that says "please don't rely on the default value", but I think Hyrum's Law would apply anyway :(
  2. Since merging Move lock logic to fakeClock. Reduce Ticker and Timer code complexity. #52, we haven't seen any issues/bugs on this yet. Although breakage is a concern, it hasn't been brought up yet. I admit I secretly hope it encourages people to migrate to NewFakeClockAt :)
  3. We could revert the default change from Move lock logic to fakeClock. Reduce Ticker and Timer code complexity. #52. Theoretically there could be breakage from reverting the default as well, although reasonably less than keeping it I suppose. I don't know how one reasons about this in open source.

I'm open to whatever you decide, just let me know.

@sagikazarmark
Copy link
Collaborator

As you said, the cat is already out of the bag, so let's at least try to help people by adding a more explicit warning to NewFakeClock's documentation.

Other than that (and a rebase), LGTM.

@DPJacques
Copy link
Collaborator Author

Done. Let me know if that works. Happy to modify it as needed.

@DPJacques
Copy link
Collaborator Author

After we get this merged, would it be possible to upgrade the version from 0.3.0 to 0.4.0?

This prevents callers from building tests that rely on implementation
details. Typically by expecting a string containing a static, default
value of NewFakeClock().

This philosophy of preventing users from relying on implementation
internals is similar to Go's random map iteration behavior.

For users who depend on the initial clock time, NewFakeClockAt provides
the behavior they need, and makes their expectations clear in tests.
@DPJacques DPJacques changed the title Use a random time for NewFakeClock's initial value Use the current time for NewFakeClock's initial value Apr 1, 2023
@sagikazarmark
Copy link
Collaborator

After we get this merged, would it be possible to upgrade the version from 0.3.0 to 0.4.0?

Sure, no problem.

I can see that you changed the fake clock initial value to the current time. Any particular reason why?

I can't say why, but it sounds somewhat better than an arbitrary random value.

@DPJacques
Copy link
Collaborator Author

DPJacques commented Apr 1, 2023

I was motivated by what I thought was being requested in #61, although I now understand that issue to be asking for something else. I pondered it a bit this morning, and using time.Now() seems less complex while still accomplishing the desired goal.

Sorry for the late change with no comment beforehand. I was responding to #61. Maybe I should have commented first before pushing. My apologies for that.

@sagikazarmark
Copy link
Collaborator

No worries!

I believe this is a better solution, we just needed some time to find it.

@sagikazarmark sagikazarmark added the release-note/breaking-change Release note: Breaking Changes label Apr 1, 2023
Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sagikazarmark sagikazarmark merged commit 606c48b into jonboulle:master Apr 1, 2023
ahrtr added a commit to ahrtr/etcd that referenced this pull request Apr 11, 2023
@DPJacques DPJacques deleted the rand branch April 14, 2023 21:54
chmouel added a commit to chmouel/pipelines-as-code that referenced this pull request May 10, 2023
clockwork has a new behaviour, cf:
jonboulle/clockwork#55 so fixing those  as well.

bump to github/v52, i have a commit in there that fix a workaround i had to add
for tkn pac info.

wait.PollImmediate is deprecated, adding a nolint for now until wel this is
removed :upside_down:

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
chmouel added a commit to chmouel/pipelines-as-code that referenced this pull request May 11, 2023
make minimal requirement to go 1.19

clockwork has a new behaviour, cf:
jonboulle/clockwork#55 so fixing those  as well.

bump to github/v52, i have a commit in there that fix a workaround i had to add
for tkn pac info which can be removed.

wait.PollImmediate is deprecated, adding a nolint for now until wel this is
removed :upside_down:

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
piyush-garg pushed a commit to openshift-pipelines/pipelines-as-code that referenced this pull request May 11, 2023
* update all docker image to latest

latest go-toolset and 311 python

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

* Update all dependencies

make minimal requirement to go 1.19

clockwork has a new behaviour, cf:
jonboulle/clockwork#55 so fixing those  as well.

bump to github/v52, i have a commit in there that fix a workaround i had to add
for tkn pac info which can be removed.

wait.PollImmediate is deprecated, adding a nolint for now until wel this is
removed :upside_down:

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

---------

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/breaking-change Release note: Breaking Changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants