Skip to content
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

Fixed applying of the root verification rules #502

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

shanshin
Copy link
Collaborator

@shanshin shanshin commented Nov 6, 2023

Now, when creating the Android report configuration, on Check is immediately set for verification.

This is not necessary, because if not specified, only the koverVerify task is started when executing check, but not the tasks of verifying Android build variant. This has a negative side effect, because the verification for the build variant is always specify, and it will override the common rules from the root of the report config koverReport { verify { /* common rules */ } }

Fixes #459

Now, when creating the Android report configuration, on Check is immediately set for verification.

This is not necessary, because if not specified, only the `koverVerify` task is started when executing check, but not the tasks of verifying Android build variant.
This has a negative side effect, because the verification for the build variant is always specify, and it will override the common rules from the root of the report config `koverReport { verify { /* common rules */ }  }`

Fixes #459
@shanshin shanshin requested a review from sandwwraith November 6, 2023 17:14
@sandwwraith
Copy link
Member

onCheck is immediately set for verification.

Is immediately set to false, judging by the edited code?

This is not necessary, because if not specified

If what is not specified?

because the verification for the build variant is always specify

Is specified by default, you mean?

I'm not sure I've understood the fix completely: #459 was about the rules from default koverReport { verify { ... }} being ignored when running koverVerify{VariantName}. How exactly setting onCheck to true will fix this issue? Now koverVerify{VariantName} will also run standard koverVerify? Or check task will run all koverVerify{VariantName}?

@shanshin
Copy link
Collaborator Author

shanshin commented Nov 8, 2023

How exactly setting onCheck to true will fix this issue?

No, onCheck does not become true, it stops being filled in at all.
According to the rules for Android reports, if onCheck is not specified, it is taken as false.

Now, to install onCheck, the reports.verify { block is called.
if you look at its implementation, you can see that the default value for verify config is null.
When a user writes verify { }, according to the current rules, it automatically resets all filters declared in the hierarchy above.

The problem was that the onCheck setting took place through a verify block call, which should be filled in by the user, so the filters seem to be erased.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

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

This explanation makes much more sense, thank you. Can you add it to the commit message?

@shanshin shanshin merged commit b3ca174 into main Nov 14, 2023
@shanshin shanshin deleted the fix-root-verification-rules branch November 14, 2023 12:09
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.

Top level verification rule is not applied to verification tasks for Android build variants
2 participants