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

Move market volume test to the appropriate package #251

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

snagai
Copy link
Contributor

@snagai snagai commented Aug 16, 2024

#248

This pull request includes the following changes:

  • Moved test from backend/tests/market_math/marketvolume_test.go to backend/handlers/math/market/marketvolume_test.go
  • Updated the variable name in the loop to follow the table-driven test naming convention.
  • Added description comments to the test function TestGetMarketVolume

@snagai snagai marked this pull request as ready for review August 16, 2024 14:12
@pwdel pwdel requested review from pwdel and ajlacey August 16, 2024 17:20
@pwdel
Copy link
Member

pwdel commented Aug 16, 2024

Greetings @snagai thanks for your contribution. May I ask how you found us and what your interest is in SocialPredict?

I would love to invite you to our Discord to chat more. If possible, it would be great to perhaps set up a call so I can understand more about what you are interested in as far as your open source contribution and how it might either help your career, hobbies or whatever you plan to do.

So basically I just had a talk with @ajlacey, a new contributor on this project, who is helping a friend get SocialPredict ready for a University Class in the United States. Just yesterday we talked about putting together a whole project which would comprehensively refactor Golang to use standard style.

I am curious your rationale in moving the test into the function file rather than within its own file?

Please join our discord here if you would like.

https://discord.gg/vtGQBwwd

Here is our Golang refactoring project.

https://github.com/orgs/openpredictionmarkets/projects/6

@pwdel
Copy link
Member

pwdel commented Aug 16, 2024

@snagai and @ajlacey as a discussion point for the larger issue, I put together a ticket here:

https://github.com/orgs/openpredictionmarkets/projects/6/views/1?pane=issue&itemId=75100490

@pwdel
Copy link
Member

pwdel commented Aug 16, 2024

OK I can see after having read that Go recommends you put tests within the package folders.

https://go.dev/doc/tutorial/add-a-test

@pwdel
Copy link
Member

pwdel commented Aug 16, 2024

Whoops sorry @snagai I am behind on this project. I did not realize you had already reached out and communicated with @ajlacey and I am not as experienced of a Golang developer. Thanks for your contribution. That being said, would love to talk with you further if you are interested.

@pwdel pwdel merged commit 8fea7a1 into openpredictionmarkets:main Aug 16, 2024
@ajlacey
Copy link
Collaborator

ajlacey commented Aug 16, 2024

Closes #248

Copy link
Collaborator

@ajlacey ajlacey left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks @snagai !
Closes #248

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

Successfully merging this pull request may close these issues.

3 participants