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.

Sign up for GitHub

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

Jump to bottom

Test: Internal exchange coverage #1525

Merged
merged 2 commits into from
May 7, 2024
Merged

Test: Internal exchange coverage #1525

merged 2 commits into from
May 7, 2024

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Apr 19, 2024 •
edited
Loading

Minimal test coverage for internal/testing/exchange.
It doesn't cover all statements or methods, but at least gets us started in the right direction.

Type of change

  • Test coverage improvement

How has this been tested

  • go test ./... -race
  • golangci-lint run

@gbjk gbjk self-assigned this Apr 19, 2024
@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch from e226709 to 9a9f9d1 Compare April 19, 2024 05:10
@codecov Codecov
Copy link

codecov bot commented Apr 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.18182% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 36.01%. Comparing base (ece58eb) to head (9dd97f6).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
- Coverage   36.01%   36.01%   -0.01%     
==========================================
  Files         411      412       +1     
  Lines      177760   177779      +19     
==========================================
+ Hits        64025    64030       +5     
- Misses     105881   105894      +13     
- Partials     7854     7855       +1
Files Coverage Δ
internal/testing/exchange/exchange.go 50.80% <62.50%> (-13.90%) ⬇️
internal/testing/utils/path.go 71.42% <71.42%> (ø)

... and 15 files with indirect coverage changes

@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch from 9a9f9d1 to d2a1c17 Compare April 19, 2024 05:59
@thrasher- thrasher- added the review me This pull request is ready for review label Apr 19, 2024
gloriousCode
gloriousCode requested changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

Who tests the test functions? Thanks for adding additional confidence to these helpers

internal/testing/exchange/exchange.go Outdated Show resolved Hide resolved
internal/testing/exchange/exchange_test.go Outdated Show resolved Hide resolved
@gbjk gbjk requested a review from gloriousCode April 28, 2024 03:00
gloriousCode
gloriousCode requested changes Apr 28, 2024
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

Thanks for making that change and the explanations.

I will ask you to look at the build issues here:
https://github.com/thrasher-corp/gocryptotrader/actions/runs/8864549340/job/24339848689?pr=1525#step:9:291
It looks like ARM64 Windows is unable to work with your changes. This also impacts any exchange that utilises this eg Binance

The macos build failure appears to be a separate issue

@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch 6 times, most recently from eeee562 to 66741a6 Compare April 29, 2024 14:21
@gbjk
Copy link
Collaborator Author

gbjk commented Apr 29, 2024

I will ask you to look at the build issues here: https://github.com/thrasher-
It looks like ARM64 Windows is unable to work with your changes.

Fixed 66741a6

shazbert
shazbert requested changes Apr 29, 2024
View reviewed changes
Copy link
Collaborator

@shazbert shazbert 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, I like the name change. Just one thing.

internal/testing/utils/path_test.go Outdated Show resolved Hide resolved
@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch from 276f2b9 to c92e04a Compare April 30, 2024 01:46
@gbjk gbjk requested review from gloriousCode and shazbert April 30, 2024 01:47
shazbert
shazbert approved these changes Apr 30, 2024
View reviewed changes
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

Thanks looking good.

@shazbert shazbert added the szrc shazbert review complete label Apr 30, 2024
thrasher-
thrasher- requested changes May 3, 2024
View reviewed changes
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

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

Thank you for enhancing the test coverage! Just a minor nit

internal/testing/utils/path.go Outdated Show resolved Hide resolved
@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch from 87a1919 to db49415 Compare May 3, 2024 07:58
@gbjk gbjk requested a review from thrasher- May 5, 2024 05:31
gloriousCode
gloriousCode requested changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes 🎉

internal/testing/utils/path_test.go Outdated Show resolved Hide resolved
gbjk added 2 commits May 7, 2024 11:53
@gbjk
Test: Internal exchange coverage
2d38ab4
@gbjk
Tests: Rename exchange.TestInstance to Setup
9dd97f6 
This package function is either going to be imported and used as just exchange.Setup,
or more likely testexch.Setup.
This removes the Stutter of having
internal/testing/exchange.TestInstance which is implicit given the
package path
@gbjk gbjk force-pushed the bugfix/internal_exchange_cov branch from 46a9d14 to 9dd97f6 Compare May 7, 2024 04:54
gloriousCode
gloriousCode approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

tACK!

thrasher-
thrasher- approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

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

Thank you for making those changes!

@thrasher- thrasher- merged commit 93c2d01 into thrasher-corp:master May 7, 2024
10 of 13 checks passed
@gbjk gbjk deleted the bugfix/internal_exchange_cov branch May 7, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shazbert shazbert shazbert approved these changes

@gloriousCode gloriousCode gloriousCode approved these changes

@thrasher- thrasher- thrasher- approved these changes

Assignees

@gbjk gbjk

Labels
review me This pull request is ready for review szrc shazbert review complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gbjk @thrasher- @gloriousCode @shazbert