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

Tests are deterministic #88

Open
Streppel opened this issue Jul 24, 2019 · 2 comments
Open

Tests are deterministic #88

Streppel opened this issue Jul 24, 2019 · 2 comments

Comments

@Streppel
Copy link
Collaborator

Streppel commented Jul 24, 2019

Current implementation depends too much on using time manipulation and because of that some tests take a long time to run (testTaskAt is one example, as it depends on the system changing minute, even though I believe we could fix this by testing it differently. Another example is the recent locking implementation).

This also causes some weird bugs that happen only at certain times (I've seen cases where time.Now().Minute() + 1 was used to compare times, causing failures if you ran the test suite anytime at HH:59 -> edit to clarify, I tend to fix these when I find them :))

We should find a way to end this by wrapping the system clock and thus being able to mock and control it.

Further reading

@JohnRoesler
Copy link
Contributor

Regarding handling time - we could make an interface and use that and then mock it when we test

package time

import "time"

type Time interface {
	Now() time.Time
}

type GoTime struct{}

func (t GoTime) Now() time.Time {
	return time.Now()
}

@Streppel
Copy link
Collaborator Author

Streppel commented Nov 15, 2019

Absolutely @JohnRoesler, we will need to wrap our clock and using an interface for this is the way to go.

Currently, we use Unix() time to measure time difference instead of .After() and .Before() functions because these last two count differences up to nanoseconds, which used to cause bugs on crons that ran every second. Using Unix time, we only work with seconds. I think that if we stick to using Unix() to measure our time difference, this should be explicit somehow in the interface.

I'm really into this. It would make our tests more reliable and faster (and we currently need that)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants