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

Restructure LCD Tests #1157

Closed
rigelrozanski opened this issue Jun 7, 2018 · 8 comments
Closed

Restructure LCD Tests #1157

rigelrozanski opened this issue Jun 7, 2018 · 8 comments
Labels

Comments

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jun 7, 2018

LCD test has some confusing architecture to it which should be rectified. The current process flow is to globally call a setup function in a TestMain function which affects global variables which are in turn used throughout all the tests. This organization is really confusing to debug, what should be happening is we should remove both the use of the TestMain function and global variables all together, and instead have each Test initialize state from the helper function at the beginning of each test (or for the tests where this is necessary) and have this initialization function pass whatever parameters are necessary back in the function return (replacing the use of global variables).
^ The above change will clear a lot of confusing referencing currently happening in the LCD

We should also:

  • replace references to Gaia app with mock application
  • remove hard coded json snippets used in the process flow
@cwgoes
Copy link
Contributor

cwgoes commented Jun 20, 2018

Another problem with the current architecture: #1296 - the LCD send endpoint was broken because our LCD tests run the LCD in-process, and we use viper.Set - which doesn't replicate the actual usage, where the LCD and queries are run in separate processes and don't share a Viper state...

@cwgoes cwgoes mentioned this issue Jun 20, 2018
6 tasks
@rigelrozanski
Copy link
Contributor Author

i "love" viper

@rigelrozanski
Copy link
Contributor Author

Also I take it back, after talking with Fabo I think it's good to have hard coded snippets in, as it informs the users of the LCD that the interface has changed

@ValarDragon
Copy link
Contributor

ValarDragon commented Jun 26, 2018

Just profiled this, 76% of the time is spent on generating the key from password. (Bcrypt doing its job as a slow hash) We are making 28 calls to bcrypt. I think we should see if we can reduce this number. I think we should reduce the security parameter of bcrypt for testing, we don't gain much by delaying all test suites for Bcrypt. I recall discussion that we are going to be moving the keys out of go-crypto, so I guess fixing the testing speed is blocked until that gets into the sdk.

@rigelrozanski
Copy link
Contributor Author

Oh cool - yeah I know how we can reduce this to one or two times! Great analysis to know thanks

@ebuchman
Copy link
Member

ebuchman commented Jul 4, 2018

@rigelrozanski this was mostly done? Should we close and follow up with more particular issues about this package, like #1551

@rigelrozanski
Copy link
Contributor Author

Yes - will open some new issues

@rigelrozanski
Copy link
Contributor Author

ref: #1553

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants