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

services/ticker: TestInsertOrUpdateAsset random failure #2063

Closed
bartekn opened this issue Dec 16, 2019 · 5 comments
Closed

services/ticker: TestInsertOrUpdateAsset random failure #2063

bartekn opened this issue Dec 16, 2019 · 5 comments

Comments

@bartekn
Copy link
Contributor

bartekn commented Dec 16, 2019

TestInsertOrUpdateAsset failed when testing a commit that was a markdown file edit: https://circleci.com/gh/stellar/go/12298

@leighmcculloch
Copy link
Member

leighmcculloch commented Dec 16, 2019

This test used to flake more frequently and I thought I fixed it by replacing use of Truncate with Round. That was in this commit: 3e23070. The fix wasn't perfect, Postgres and Go round time differently:

The fix in this change isn't perfect according to discussion at lib/pq#227 (comment), where a commenter states that Postgres' rounding function rounds to nearest and in the event of a tie rounds to even, which is different to Go's Round function which rounds to nearest and in the event of a tie rounds away from zero.

I'll see if I can find a better fix.

The Go lib/pq issue relating to this is lib/pq#227.

@leighmcculloch
Copy link
Member

leighmcculloch commented Dec 16, 2019

I've opened issue #2064 to add test logs to give us 👀 into what time inputs can be used to reproduce the issue. I won't attempt to fix the issue right now as the time cost to fix it without knowing what inputs it is failing on will be high. Next time it fails with these test logs report here and I'll fix it.

Hindsight: I wish I had added these test logs when I added the last fix.

leighmcculloch added a commit that referenced this issue Dec 16, 2019
#2064)

Add logs to the `TestInsertOrUpdateAsset` that print out the exact times that are being used.

The test has been reported as flaky a couple times, first in #1733 then in #2063. There was an imperfect fix put in place in 3e23070 but we've seen another test failure. Previously when I debugged this I wrote some test cases to try long series of inputs to find an input that caused a failure. This time I think it will be a better use of our time if we improve the test logs in this test so that the next time it fails we know exactly what inputs are being used for time and then we can address the continued flakyness.

This doesn't fix the issue reported, just gives us more visibility into a set of inputs we can reproduce with so that when we spend time fixing it we can be confident if we have fixed it or not.
@leighmcculloch
Copy link
Member

To my knowledge these tests haven't flaked since this issue was opened, or since logs were added in #2064. The logs are there to help us fix the issue if or when it happens again. I'll close this issue now. It can be reopened or a new issue created if this test flakes again.

@bartekn
Copy link
Contributor Author

bartekn commented May 31, 2021

@bartekn
Copy link
Contributor Author

bartekn commented Aug 30, 2021

@bartekn bartekn reopened this Aug 30, 2021
@leighmcculloch leighmcculloch removed their assignment Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants