-
Notifications
You must be signed in to change notification settings - Fork 17
Add new config param 'show_reproduce_content' #116
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
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.
Thanks for the patch!
Please, consider two comments below.
lib/test_suite.py
Outdated
@@ -214,3 +217,11 @@ def is_parallel(self): | |||
raise ConfigurationError() | |||
pass | |||
return val | |||
|
|||
@property | |||
def show_reproduce_content(self): |
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.
Please, raise an error for anything except True
, False
or omitted option.
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.
I mean, we have two boolean parse code: here and in 'is_parallel`. Sorry, I don’t write it explicitly. I think one the versions of the code should be reused.
lib/worker.py
Outdated
@@ -177,7 +179,7 @@ def done_marker(self): | |||
return WorkerDone(self.id, self.name) | |||
|
|||
def wrap_result(self, task_id, short_status): | |||
return WorkerTaskResult(self.id, self.name, task_id, short_status) | |||
return WorkerTaskResult(self.id, self.name, task_id, short_status, self.suite.ini) |
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.
It looks too expensive to pass full test suite configuration over the pipe per each test result. Please, extract the option from self.suite
.
It is possible to omit passing the option value from worker processes to the main process at all. We need to pass the option information from task groups when creating StatisticsWatcher
(see dispatcher.py:127) and use worker_name
part after _
(underscore) in StatisticsWatcher.process_result
to determine the test suite name and so the option value. The approach looks better for me, because it doesn't pass data already known by the main process, but I don't insist on it. It has obvious disadvantage: we lean on the worker naming schema.
153a238
to
91819ef
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.
One minor comments added.
No need to re-review again with me, LGTM.
lib/test_suite.py
Outdated
@@ -214,3 +217,11 @@ def is_parallel(self): | |||
raise ConfigurationError() | |||
pass | |||
return val | |||
|
|||
@property | |||
def show_reproduce_content(self): |
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.
I mean, we have two boolean parse code: here and in 'is_parallel`. Sorry, I don’t write it explicitly. I think one the versions of the code should be reused.
By default this param is True, if the test fails then the contents of reproduce file are displayed. If the parameter is False, do not show the contents of reproduce file. For example, the 'sql-tap' tests run each case in a separate tarantool instance, so reproduce is not necessary for problem investigation. Used in 'suite.ini'. Closes #113
91819ef
to
4a0340e
Compare
By default this param is True, if the test fails then the contents of
reproduce file are displayed. If the parameter is False, do not show
the contents of reproduce file. For example, the 'sql-tap' tests run
each case in a separate tarantool instance, so reproduce is not
necessary for problem investigation. Used in 'suite.ini'.
Closes #113