search

surrealdb / surrealdb.go

SurrealDB SDK for Golang
https://surrealdb.com
Apache License 2.0
231 stars 62 forks source link

Goreport added. #70

Closed ElecTwix closed 1 year ago

ElecTwix commented 1 year ago

Goreport added for code quality display

timpratim commented 1 year ago

Hi @ElecTwix I went through your changes and I wanted to suggest some changes.

1) I feel 50 is a small number to test concurrency with and would suggest you increase the number of coroutines? To ensure high levels of concurrency. 100 seems like a good number. 2) Is it possible to change the 3 tests into a single test based on different scenarios? 3) The scenario is loop 1..100 select => no row error create select => validate 4)There is a single wait group that is shared between 3 tests, this can introduce race conditions.

ElecTwix commented 1 year ago

@timpratim Thanks for your review, I think we should review this in #69, this PR just GoReport.

  1. I feel 50 is a small number to test concurrency with and would suggest you increase the number of coroutines. To ensure high levels of concurrency. 100 seems like a good number.

That suits me.

  1. Is it possible to change the 3 tests into a single test based on different scenarios?

Far as I remember we were doing unit testing, not a stress test if that's the case I can move some tests to another file.

  1. There is a single wait group that is shared between 3 tests, this can introduce race conditions.

Unfortunately, we are using testify's test suite and we cannot run all tests like race conditions in the unit test it needs to be cleaned up after every unit test.

timpratim commented 1 year ago

Hi, @ElecTwix , thank you for your contribution to this PR. Could you please consider removing the websocket test from this particular PR? It would be beneficial to keep this PR focused on Goreport for better clarity in tracking changes.

Since the review comments have already been addressed in #69, it would be great if we could track the websocket test there instead. This will help reduce redundancy.

ElecTwix commented 1 year ago

Hi, @ElecTwix , thank you for your contribution to this PR. Could you please consider removing the websocket test from this particular PR? It would be beneficial to keep this PR focused on Goreport for better clarity in tracking changes.

Since the review comments have already been addressed in #69, it would be great if we could track the websocket test there instead. This will help reduce redundancy.

converted to draft PR when other PR has been merged then I can rebase and open the PR.

ElecTwix commented 1 year ago

@timpratim this PR ready for review.