-
Notifications
You must be signed in to change notification settings - Fork 185
Fix to HTML Coverage #326
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
Fix to HTML Coverage #326
Conversation
1 similar 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.
Thanks @lwasylow for your first contribution.
Nice catch with that bug and a really quick fix.
Minor comments on the fix itself.
It would be great if we could have a test created that would be proving the bug existence and also that the fix is working correctly.
This way we avoid regression in future, when changes will be done to this code base.
source/core/types/ut_run.tpb
Outdated
@@ -29,7 +29,8 @@ create or replace type body ut_run as | |||
self.coverage_options := a_coverage_options; | |||
self.test_file_mappings := coalesce(a_test_file_mappings, ut_file_mappings()); | |||
if self.coverage_options is not null then | |||
l_run_schemes := get_run_schemes(); | |||
l_run_schemes := get_run_schemes(); | |||
l_run_schemes := coalesce(coverage_options.schema_names, l_run_schemes); |
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 seems that this fix will work well but the variable l_run_schemes
changes it's meaning.
Before that it was holding list of schema names that are holding unit tests to run.
With this operation, it will hold the schema names that contain code to gather coverage on.
It actually seems a right thing to do but I would change the variable name.
It should be called l_coverage_schema_names
or l_schema_names_to_cover
or something like this.
This way we avoid confusion and ambiguity.
Could you also make those two lines into one with single assignment and make sure indents are consistent?
2 similar comments
1 similar comment
2 similar comments
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.
Great work!
Adding ability to overwrite default schemas list used in html code coverage.