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

ci(-race): race condition with -race flag #2663

Closed
wants to merge 22 commits into from

Conversation

DIGIX666
Copy link
Contributor

@DIGIX666 DIGIX666 commented Aug 6, 2024

-race flag

I do going to try make this issues #2097

This is my draft PR for -race flag. For moment not all files is fixet.
My plan for solve these race conditions :

  1. Run go test -race ./...

  2. Check all files in running DATA RACE and Goroutine
    image
    image

  3. Use a best technique (mutex, chan, sync/atomic) for solve Data Race and Goroutine

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 6, 2024
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Aug 6, 2024
Copy link

codecov bot commented Aug 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.85%. Comparing base (01ee5a9) to head (f0eb99a).
Report is 18 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2663   +/-   ##
=======================================
  Coverage   60.84%   60.85%           
=======================================
  Files         563      563           
  Lines       75190    75184    -6     
=======================================
  Hits        45753    45753           
+ Misses      26068    26063    -5     
+ Partials     3369     3368    -1     
Flag Coverage Δ
contribs/gnofaucet 15.31% <ø> (+0.85%) ⬆️
gnovm 65.63% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/logos 20.23% <ø> (+0.35%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DIGIX666 DIGIX666 changed the title Fix : race condition with -race flag ci : race condition with -race flag Aug 6, 2024
@DIGIX666 DIGIX666 changed the title ci : race condition with -race flag ci(-race): race condition with -race flag Aug 6, 2024
@DIGIX666 DIGIX666 marked this pull request as ready for review September 11, 2024 10:41
@DIGIX666
Copy link
Contributor Author

DIGIX666 commented Sep 11, 2024

I am currently correcting the test errors on the bytes_test.gno, L542 and sort_test.gno, L656 files but I don't understand if // XXX remove testenv means that it is no longer used or if it should be replaced because testenv doesn't exist in gno and I wanted to replace it with os.Getenv but it doesn't exist on gno either because I wanted to use an export RACE=true in the CI test-with-race

cc @ajnavarro @thehowl

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Sep 22, 2024
@DIGIX666
Copy link
Contributor Author

DIGIX666 commented Sep 22, 2024

Flag Update

I've removed the t.Parallel() in files with concurrency but I think we should remove all t.Parallel() (I'll try before I push) to reduce the risk of race conditions but also added Mutex.

gnovm

The issue with the test go test -race -timeout 30m ./... in the gnovm folder is that there's an error every time, even without a timeout, on the file xfactor_long.gno because it panics.

panic: test timed out after 10m0s
running tests:
	TestFilesNative (10m0s)
	TestFilesNative/xfactor_long.gno (1m11s)

I tried to skip the file during the test by adding:

if filepath.Base(path) == "xfactor_long.gno" {
	t.Skip("skipping non-gno file:", path)
}

in the file eval_test.go L14

But it didn’t change anything.

For the gnovm folder, I think the -race flag is not relevant because the tests in /gnovm/tests/files are .gno files that intentionally test for created errors.

tm2

Currently for tm2, the remaining test is TestMempoolNoProgressUntilTxsAvailable, which times out, and the test TestNodeDelayedStart, which gives me the following error, but I don’t know how to fix it:

Data: failed to listen on 0.0.0.0:26657: listen tcp 0.0.0.0:26657: bind: address already in use

For the CI gno.land/Run Main/Go Test/test and tm2/Run Main/Go Test/test, I don’t understand their failures because I tested them locally and they passed.

Thank you for checking my changes when you have the time and for guiding me if possible to fix the remaining issues :)

@mvertes
Copy link
Contributor

mvertes commented Sep 27, 2024

Thank you for your contribution, but this is part where we are working on refactoring the whole set of tests, also in relation to gnovm, it's better to let this to the core team. There is a lot missing context for you, and it's up to us to clarify before welcoming contributions on this specific part, thanks.

@mvertes mvertes closed this Sep 27, 2024
@DIGIX666
Copy link
Contributor Author

Thank you for your contribution, but this is part where we are working on refactoring the whole set of tests, also in relation to gnovm, it's better to let this to the core team. There is a lot missing context for you, and it's up to us to clarify before welcoming contributions on this specific part, thanks.

Thanks for your feedback and explanation I understand what you mean. In any case, it would have allowed me to learn something new 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related 📦 🤖 gnovm Issues or PRs gnovm related
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants