-
Notifications
You must be signed in to change notification settings - Fork 59
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
Clean up perf to be able to run against env #1600
Comments
Note that @ayrat555 has been working on this as part of omgnetwork/omg-childchain-v2#59 One PR: #1691 |
Hihi @ayrat555 , I am sorry to be that person, but can I ask more on the PR: #1691 My main concern is what and why we are maintaining as @InoMurko pointed out. After the PR, it seems like only 2 main suite of tests are left: “byzantine” and “extended perf test”. I believe extended perf test is already replaceable via the childchain tests or the utxo test in original perf setup. (It is sending a bunch of tx to childchain server directly right?). I think it is quite fair and remove that. (cc. @kevsul please help double check on this point) The thing perf does not have is testing with multiple exits, and we previously had an incident that our service was actually poor at handling multiple exit events. So it probably makes sense to have tests that auto spins up certain spike amount of exits. But I would strongly recommend to port that using the same style of perf original setup where it hit the env instead of relying on docker geth accounts. I recall @kevsul had been working on this previously, what was your latest status, is there any work that can be reusable?? |
I am reopening this. @ayrat555 if this is too much to ask to cleanup (and I totally understand you are not the person to ask...you are just some poor guy that comes into this) I think it is fine and fair to keep the remaining to gold team but I will leave this decision to you. |
Perhaps it makes sense to open a new issue with the new requirements, rather than opening this one? |
Yeah, the original purpose of this issue was to port only the byzantine_events_test into the Chaperon perf test framework. Apologies for not catching this sooner @ayrat555 , I didn't get around to looking at the PR until today. I did make a start on this a while back, but I didn't get half as far. I think we can build on @ayrat555's work though - we just need to run the tests via the Chaperon framework.
(similarly for IFEs, but let's leave that as a separate task) |
I would say that @ayrat555 did what the ticket says it should be done. There's no mention of Chaperon. The PR he did decoupled those tests with the elixir-omg code, which is great. We either rename this ticket so that the requirement is chaperon of newly added tests, or close this one and open a new one. |
That was the unfortunate part as the description of the issue is empty😂😂so everyone are not in the same page on what the requirements should be. I still want to give big 👏👏to @ayrat555 as that it was the ball that everyone is throwing or waiting some brave guy to take😛 To move forward I just want to make sure it is still tracked, no preference in what form |
No description provided.
The text was updated successfully, but these errors were encountered: