Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

CI race detector for Swarm #1205

Closed
wants to merge 5 commits into from
Closed

CI race detector for Swarm #1205

wants to merge 5 commits into from

Conversation

frncmx
Copy link
Contributor

@frncmx frncmx commented Feb 8, 2019 •
edited
Loading

We had more than a dozen of Data Races in our code base. Go has a nice builtin race detector tool. So we should have a CI job that at least notifies us timely if we introduce a new race.

1st let's just have a daily CRON job triggered on ethersphere/go-ethereum. Later as the job gets stable, we can start using it for ethersphere PRs.

resolves: #741

Known Issues

@frncmx frncmx mentioned this pull request Feb 8, 2019
Closed
@nonsense
Copy link
Contributor

nonsense commented Feb 8, 2019

Geth team meetup is next week. I think we better wait for that and split the codebase, rather than pollute go-ethereum/.travis.yml with repo-specific config.

I am not sure this will be approved upstream, and is not up to us.

acud
acud approved these changes Feb 13, 2019
View reviewed changes
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Nicely done. I'd add the race detector to p2p and cmd/swarm too

.travis.yml Show resolved Hide resolved
@frncmx
Copy link
Contributor Author

frncmx commented Feb 20, 2019

If ethereum/go-ethereum#19143 gets merged, then the
race detector job will be faster than the other tests jobs. => We can run it on every PR on Ethersphere. (Until we are not 100% sure it's stable I would not run it on Ethereum.)

image

Note: about keepalive

Currently, looking at the test output it seems we might not need the keepalive script if we keep -v on for tests. The only problematic test is TestRefHasher. If that would run faster, then we don't need the script as we just need to produce some output every 10 minutes so Travis won't abort the build.

I guess that test and the ones coming after do not produce output because a lot of tests runs in parallel and output can be published only after the tests pass or fail.

What do you think? Should we keep the keepalive script or change tests?

ok  	github.com/ethereum/go-ethereum/swarm/api/http	4.024s
keepalive
keepalive
=== RUN   TestRefHasher

@frncmx frncmx mentioned this pull request Feb 20, 2019
Closed
2 tasks
@frncmx
Copy link
Contributor Author

frncmx commented Feb 20, 2019

@nonsense as the code split is undecided and I want to close this issue down, I want to progress and submit upstream.

I'm fine with dropping the ethereum restrictions if requested. Having the ethersphere restriction in place is everybody's interest though.

@frncmx
Copy link
Contributor Author

frncmx commented Feb 20, 2019 •
edited
Loading

I did a rebase/cleanup just to keep the upstream PR cleaner, as we have to interact with others on this.

@frncmx frncmx requested a review from zelig February 20, 2019 19:55
build: add travis_keepalive.sh helper script
c25f9dc 
`go test -race ./swarm...` sometimes fails because there is no
output produced for 10 minutes and Travis just aborts the build.
As `t.Log()` holds the buffer until the test does not pass or fail.
Ferenc Szabo added 4 commits February 20, 2019 20:59
travis: add race detector job for Swarm
627f0a9 
We had more than a dozen of Data Races in our code base. Go has
a nice builtin race detector tool. So we should have a CI job
that at least notifies us timely if we introduce a new race.

1st let's just have a daily CRON job triggered on
ethersphere/go-ethereum. Later as the job gets stable, we can start
using it for ethersphere PRs.

resolves: #741
travis: run Ubuntu PPA upload job only on ethereum/go-ethereum
562f3f2 
As we only deal with artifacts in the upstream repo.
travis: run Azure related jobs only on ethereum/go-ethereum CI
9a4069d 
As we only deal with artifacts there and secretes are not even
available on our forks.
travis: remove cron restriction from Swarm race detector job
2a41455 
As the completion time is around ~15m currently, so we can run
it against every push/PR. Still, just on ethersphere.
@frncmx frncmx requested a review from janos February 20, 2019 19:59
@frncmx
Copy link
Contributor Author

frncmx commented Feb 21, 2019

upstream: ethereum/go-ethereum#19148

Moved there as we need reviews from the geth team, too.

@frncmx frncmx closed this Feb 21, 2019
@frncmx frncmx deleted the ci-race-detector branch February 22, 2019 13:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acud acud acud approved these changes

@nonsense nonsense Awaiting requested review from nonsense

@skylenet skylenet Awaiting requested review from skylenet

@zelig zelig Awaiting requested review from zelig

@janos janos Awaiting requested review from janos

Assignees
No one assigned
Labels
stability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create a separate CI job for race detection
3 participants
@frncmx @nonsense @acud