[java] New rule: AvoidAssert#3488
Conversation
adangel
left a comment
There was a problem hiding this comment.
Thanks for the PR!
I'm not sure, whether the rule is useful in it's current state. I agree with your first point, that for unit tests, the assert statement should not be used, since it can be disabled.
But for production code, this is different: I think, that assert statements are used to check pre-conditions / post-conditions. And in production, the asserts are often not enabled (since it makes the program run slower). But for running unit tests, these are typically enabled (at least maven-surefire-plugin does it that way). So you would notice, when your pre/post-conditions fail during unit tests.
The good point about assert statements is, that they indeed can be disabled. But then again, they must not have any side-effect. I think, you can just use an arbitrary expression that evaluates to a boolean as an assert expression, so you could potentially write something like
assert i++ == 0
I currently see two ways, to improve the rule:
- detect, whether the compilation unit is a test case --> then any assert statement should be banned
- if it's not a test case, then check, whether the condition has any side-effects (like calling a method, assigning a variable). In that case, the assert should be flagged.
Otherwise, we wouldn't allow any valid usages of the assert statement at all...
| This release ships with 1 new Java rule. | ||
|
|
||
| * java-errorprone | ||
| * [`AvoidAssert`](https://pmd.github.io/pmd-6.38.0-SNAPSHOT/pmd_rules_java_errorprone.html#avoidassert) reports usages of assert. An if statement is a better choice since it cannot be disabled and hence reduces the need for 1 test case. |
There was a problem hiding this comment.
We use here a own liquid tag to link to rules: {% rule java/errorprone/AvoidAssert %}
See also https://pmd.github.io/latest/pmd_devdocs_writing_documentation.html#custom-liquid-tags
Generated by 🚫 Danger |
|
I did not check off "Complete build ./mvnw clean verify passes" since it appears to be automated. However, I did do this. In the future, shall I check this off? |
|
I figured I might need to add some properties. The first property I propose is The second property I propose is The third property I propose is What do you think? |
This is exactly my sentiment. I think we should have a rule "SideEffectInAssert" to flag side effects, but pure asserts are absolutely fine and we shouldn't track them down with a generic AvoidAssert. My suggestion would be to have a rule SideEffectInAssert, and another rule to flag asserts just in tests (maybe named "UseTestFrameworkAssertion") instead of AvoidAssert.
I think this justification is pretty thin, since I would say every test runner out there uses
I think there's no need to have a property for this. We could just use a list of annotations that belong to those test frameworks, in line with some other rules about testing.
I think this can be achieved by configuring rulesets. I don't like the idea of adding ad-hoc file filters to rules |
This is a checklist for you and us: If you tick this, you say "it worked on my machine, I tested it...". Since you are a first-time contributor, you pull request must first be approved by one of us, before it is executed on github actions, see https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks for background info. |
Since this is a new rule, new violations are expected. |
There is indeed a test runner, that doesn't enable assertions by default (for whatever reason...): https://www.eclipse.org/tycho/sitedocs/tycho-surefire-plugin/test-mojo.html#enableAssertions The usual maven surefire plugin however, does this by default: https://maven.apache.org/surefire/maven-surefire-plugin/test-mojo.html#enableAssertions
Yes, that's the reason, why you should use a more sophisticated Assert class to report a failing test. In summary, I think, this rule should be named something like "AssertStatementInTests" and the rule should only report assert statements in test classes. The category here would be best practices. And maybe, we should have a separate rule, that finds assert statement in both production code and test code, that have side effects (e.g. "AssertStatementWithSideeffect"). This would belong to the category error prone. |
oowekyala
left a comment
There was a problem hiding this comment.
@nathanspectacular I split off #3777 and #3776 to track new rules about assertions. You're welcome to rework this PR to eg implement #3777 and submit it again. I'll be happy to review it and help.
Describe the PR
Adds a rule called AvoidAssert.
For test code, use the JUnit or TestNG Assert class since it cannot be disabled like the assert keyword.
For production code, it requires 3 test cases to cover all scenarios:
An if statement is a better choice since it cannot be disabled and hence reduces the need for 1 test case.
Ready?
./mvnw clean verifypasses (checked automatically by github actions)Note: This is my first pull request for PMD and Github. This rule is very simple so that I can learn the process. Please let me know what else I need to do. If you feel I need to enhance the rule, please let me know.