Skip to content

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

Merged
merged 8 commits into from
May 23, 2017
Merged

Fix to HTML Coverage #326

merged 8 commits into from
May 23, 2017

Conversation

lwasylow
Copy link
Member

Adding ability to overwrite default schemas list used in html code coverage.

@coveralls
Copy link

coveralls commented May 19, 2017

Coverage Status

Coverage increased (+0.003%) to 91.752% when pulling 6bd7b12 on lwasylow:develop into e8aa109 on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 91.752% when pulling 6bd7b12 on lwasylow:develop into e8aa109 on utPLSQL:develop.

Copy link
Member

@jgebal jgebal left a 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.

@@ -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);
Copy link
Member

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?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ab0fe52 on lwasylow:develop into e8aa109 on utPLSQL:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ab0fe52 on lwasylow:develop into e8aa109 on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ab0fe52 on lwasylow:develop into e8aa109 on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ab0fe52 on lwasylow:develop into e8aa109 on utPLSQL:develop.

1 similar comment
@coveralls
Copy link

coveralls commented May 23, 2017

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ab0fe52 on lwasylow:develop into e8aa109 on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ecb9ac9 on lwasylow:develop into e8aa109 on utPLSQL:develop.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ecb9ac9 on lwasylow:develop into e8aa109 on utPLSQL:develop.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 91.708% when pulling ecb9ac9 on lwasylow:develop into e8aa109 on utPLSQL:develop.

Copy link
Member

@jgebal jgebal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

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

Successfully merging this pull request may close these issues.

4 participants