-
Notifications
You must be signed in to change notification settings - Fork 164
fix(venom): user executors performance issues #831
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
Conversation
e0b363e to
ed1d7c0
Compare
ed1d7c0 to
500527b
Compare
41c1ee8 to
90bba73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, test cases using user-defined executors execution time decreased by about 65% (mostly due to parsing improvements)
|
CDS Report build-venom-a#253.0 ✘
|
|
CDS Report build-venom-a#253.1 ✘
|
yesnault
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alexGNX Thank you for this pr.
Can you check the user_executor_many.yml test please, it's currently failing:
❯ venom run -vv user_executor_many.yml
[trac] writing venom.0.log
• Many user executor (user_executor_many.yml)
• info
• exec PASS
[info] value foo vars foo_from_vars
[trac] writing user_executor_many.info.testcase.1.step.1.0.dump.json
• mytc
• userExecutorA FAIL
Testcase "userExecutorB", step #2-0: Assertion "result.systemout ShouldEqual input.foo foo_from_vars_on_b; fooFromEnv foo_from_vars" failed. expected: input.foo foo_from_vars_on_b; fooFromEnv foo_from_vars got: input.foo defaultValueA_on_b; fooFromEnv foo_from_vars (user_executor_many.yml:0)
Testcase "userExecutorA", step #2-0: executor "userExecutorB" failed (user_executor_many.yml:0)
Testcase "mytc", step #1-0: executor "userExecutorA" failed (user_executor_many.yml:0)
final status: FAIL
Signed-off-by: alexGNX <alexandre.gagneux12@gmail.com>
Hi @yesnault, thanks for the review. It is fixed. |
yesnault
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: alexGNX <alexandre.gagneux12@gmail.com> Signed-off-by: Tiago Peczenyj <tpeczenyj@weborama.com>
Signed-off-by: alexGNX <alexandre.gagneux12@gmail.com>
This is a fix about performance issues we encountered running testsuites with many User Defined Executors (UDE). Issue 826 was opened regarding this problem.
UDEs
Previous behavior
libfolder (let's call themcreateUserandcreateProduct), butcreateUser.yamlis not valid YAML. When running a test suite that only uses thecreateProductUDE, you get the following error message:$ venom run mytestsuite.yml executor "createProduct" is not implementedeven though
createProductis valid. This occurs because the parsing ofcreateUserfailed (even if it is not used in the test suite).libDirconfiguration option to the same value as the default one (lib/), each file of the directory will be loaded 2 times at each call ofregisterUserExecutors()New behaviour
executorandstepsare present, we load the raw file in memory and we perform a duplication check:Improvement measurement
I tested this fix on a collection of test suites totalling around 700 test cases and 120 UDEs, with some of them layered (UDE calling another UDE). Runtime has been divided by a factor of 3 (from around 35min down to around 12min)
Additional performance enhancement
execexecutor reads stdout and stderr. For exemple when loading a file into a variable with exec:With 100k lines in the file, this test step used to take more than a minute. This lowers it down to only a couple ms.
maps.Clone()to clone a map