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

Feature/unit test interestpoolsc #112

Merged
merged 12 commits into from
Jun 2, 2021
Merged

Conversation

gasparyanyur
Copy link
Contributor

No description provided.

Copy link
Contributor

@kirillt kirillt left a comment

Choose a reason for hiding this comment

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

New errors
this branch: https://www.0chain-ci.net/root/0chain/-/jobs/2516/raw
master: https://www.0chain-ci.net/root/0chain/-/jobs/2545/raw

this test case fails in master too, but with different error:

fatal error: concurrent map writes

goroutine 137 [running]:
runtime.throw(0xc77964, 0x15)
    /usr/local/go/src/runtime/panic.go:617 +0x72 fp=0xc000497578 sp=0xc000497548 pc=0x4436f2
# about 500 lines of errors with goroutines
FAIL    0chain.net/sharder/blockstore
panic: fatal error config file: Config File "sc" Not Found in "[/0chain/go/0chain.net/smartcontract/interestpoolsc/config]" [recovered]
    panic: fatal error config file: Config File "sc" Not Found in "[/0chain/go/0chain.net/smartcontract/interestpoolsc/config]"

goroutine 86 [running]:
# ...
FAIL    0chain.net/smartcontract/interestpoolsc 0.049s

Also, please note that there is massive refactoring in fees.go in another branch:
https://github.com/0chain/0chain/tree/fix/fees-test

}
ie, ok := objMap["tokens_earned"]
if ok {
var earned state.Balance
Copy link
Contributor

@kirillt kirillt Mar 16, 2021

Choose a reason for hiding this comment

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

I know there is no generics in Golang, but can we avoid copy-pasting? We can create a utility module for encoding/decoding since it looks to be quite pervasive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error fatal error config file: Config File "sc" Not Found caused because in this package there is no configuration directory.
I have already added
COPY ./config/sc.yaml $SRC_DIR/go/0chain.net/smartcontract/interestpoolsc/config/
Be sure please that you can find it in your docker.local/build.sc_unit_test/Dockerfile

I think it makes no sense to create a separate package for encoding/decoding because there, for different structures, the encoding/decoding is done in different ways.
And what don't you like here? )

Copy link
Contributor

Choose a reason for hiding this comment

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

The error I reported above is from our CI server, not from my local machine.
If some configuration needs to be made, then it should be committed.
CI server runs unit tests with ./docker.local/bin/unit_test.sh --ci.

About copy-paste — nevermind, I've already realized it is difficult to avoid it in Go :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To run the smart-contract unit test I use sc_unit_test.sh.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to merge this first then:
#127

@Sriep
Copy link
Contributor

Sriep commented Apr 9, 2021

  1. This PR adds some uint tests.
  2. It also refactors modles.go. Splitting it up into one file for each object previously defined in models.go. It also adds some refactoring to sc.go.

My question: In .2 above Does this PR fix any issues with 0chian or otherwise change its behaviour, or is it just refactoring?

Copy link
Contributor

@Sriep Sriep left a comment

Choose a reason for hiding this comment

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

TestInterestPoolSmartContract_updateVariables fails.

SmartContract: tt.fields.SmartContract,
}
if tt.shouldBeOk {
config.SetupSmartContractConfig()
Copy link
Contributor

@Sriep Sriep Apr 9, 2021

Choose a reason for hiding this comment

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

TestInterestPoolSmartContract_updateVariables fails because config.SetupSmartContractConfig is expecting a config directory with a sc.??? file in, which is missing.

Elsewhere config.SetupSmartContractConfig is only used in production code.

The sc config needs to be mocked somehow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want to mock config? this does not change the state of the application, it only sets the input parameters

Copy link
Contributor

@Sriep Sriep Apr 16, 2021

Choose a reason for hiding this comment

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

TestInterestPoolSmartContract_updateVariables fails because config.SetupSmartContractConfig is expecting a config directory with a sc.??? file in, which is missing.

Some change is needed to stop the unit test from failing at this point.

I do not want a mock config. There just needs to be some change so that the unit test works. A mock config looked like one possible solution to me, but whatever works.

This was referenced Apr 9, 2021
@gasparyanyur
Copy link
Contributor Author

  1. This PR adds some uint tests.
  2. It also refactors modles.go. Splitting it up into one file for each object previously defined in models.go. It also adds some refactoring to sc.go.

My question: In .2 above Does this PR fix any issues with 0chian or otherwise change its behaviour, or is it just refactoring?
I added unit tests for this package, so that then with any changes that lead to an error, it was clear that there was some kind of bug

@guruhubb
Copy link
Member

@Sriep @gasparyanyur is this closed?

@platsko platsko requested a review from Sriep May 1, 2021 10:06
@Sriep
Copy link
Contributor

Sriep commented May 3, 2021

Still awaiting a fix for the issue mentioned by @kirillt and @Sriep above. Plus merge conflicts. Also, it's been a while, so some confounding changes might cause problems.

@guruhubb guruhubb requested a review from platsko May 3, 2021 16:15
@platsko
Copy link
Contributor

platsko commented May 4, 2021

@gasparyanyur Please resolve the conflicts.

@guruhubb guruhubb requested a review from MurashovVen May 7, 2021 22:48
@guruhubb guruhubb requested review from Kenwes13 and removed request for platsko and MurashovVen May 19, 2021 03:50
Sriep added 3 commits June 2, 2021 16:11
…nterestpoolsc

# Conflicts:
#	code/go/0chain.net/smartcontract/setupsc/setupsc.go
#	docker.local/bin/unit_test.sh
#	docker.local/build.sc_unit_test/Dockerfile
@Sriep Sriep merged commit 0444100 into master Jun 2, 2021
@Sriep Sriep deleted the feature/unit-test-interestpoolsc branch June 2, 2021 16:08
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.

None yet

5 participants