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.

Sign up for GitHub

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

Jump to bottom

[apex] ApexUnitTestClassShouldHaveAssertsRule: Support new Assert class (Apex v56.0) #4097

Merged
merged 20 commits into from Aug 29, 2022
Merged

[apex] ApexUnitTestClassShouldHaveAssertsRule: Support new Assert class (Apex v56.0) #4097

merged 20 commits into from Aug 29, 2022

Conversation

tprouvot
Copy link
Contributor

@tprouvot tprouvot commented Aug 17, 2022 •
edited by adangel

Describe the PR

Apex v56 introduced a new Assert class: https://developer.salesforce.com/docs/atlas.en-us.240.0.apexref.meta/apexref/apex_class_System_Assert.htm#apex_class_System_Assert

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Complete build ./mvnw clean verify passes (checked automatically by github actions)
  • Added (in-code) documentation (if needed)

@tprouvot tprouvot mentioned this pull request Aug 17, 2022
Closed
@SCWells72
Copy link
Contributor

SCWells72 commented Aug 17, 2022 •
edited

Hi, @tprouvot. The product changes look good. You should also update ApexUnitTestClassShouldHaveAsserts.xml with positive test cases to verify that usages of these new assertion methods result in no reported violations.

@tprouvot
Copy link
Contributor Author

Hi, @tprouvot. The product changes look good. You should also update ApexUnitTestClassShouldHaveAsserts.xml with positive test cases to verify that usages of these new assertion methods result in no reported violations.

Hi @SCWells72,

Thanks for the suggestion !
One of the negative test became ... positive with the new class, thats why I updated it also.

SCWells72
SCWells72 suggested changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@SCWells72 SCWells72 left a comment

Choose a reason for hiding this comment

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

Just a couple of small things. Also, I'm not sure if I'll have authority to approve this for merge, but I'll at least try to provide sufficient feedback so that someone who can has less work to do to get to that point.

...net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestClassShouldHaveAssertsRule.java Outdated Show resolved Hide resolved
.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml Outdated Show resolved Hide resolved
@tprouvot tprouvot force-pushed the feature/addNewClassForAssertion branch from 2a22c60 to dc03e52 Compare August 18, 2022 15:09
SCWells72
SCWells72 suggested changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@SCWells72 SCWells72 left a comment

Choose a reason for hiding this comment

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

I think with the one requested change here it's fine IMO.

.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml Outdated Show resolved Hide resolved
@adangel adangel self-requested a review August 24, 2022 10:21
@adangel adangel changed the title [apex] ApexUnitTestClassShouldHaveAssertsRule : Add new assert methods for api 56 [apex] ApexUnitTestClassShouldHaveAssertsRule: Support new Assert class (Apex v56.0) Aug 24, 2022
adangel
adangel requested changes Aug 24, 2022
View reviewed changes
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Please have a look at my comments.
I think, we can edit #4096 to cover both rules and improve both rules with this PR.

...net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestClassShouldHaveAssertsRule.java Outdated Show resolved Hide resolved
...net/sourceforge/pmd/lang/apex/rule/bestpractices/ApexUnitTestClassShouldHaveAssertsRule.java Outdated Show resolved Hide resolved
.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml Outdated Show resolved Hide resolved
.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml Outdated Show resolved Hide resolved
.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml Outdated Show resolved Hide resolved
@adangel
Copy link
Member

adangel commented Aug 24, 2022

@SCWells72 Thanks for the initial review!

@pmd-test
Copy link

1 Message
📖 Compared to master:
This changeset changes 0 violations,
introduces 0 new violations, 0 new errors and 0 new configuration errors,
removes 0 violations, 37 errors and 6 configuration errors.
Full report

Generated by 🚫 Danger

tprouvot and others added 11 commits August 25, 2022 10:46
@tprouvot @adangel
Update pmd-apex/src/main/java/net/sourceforge/pmd/lang/apex/rule/best…
e0b9965 
…practices/ApexUnitTestClassShouldHaveAssertsRule.java

Co-authored-by: Andreas Dangel <andreas.dangel@adangel.org>
@tprouvot @adangel
Update pmd-apex/src/test/resources/net/sourceforge/pmd/lang/apex/rule…
d5098ca 
…/bestpractices/xml/ApexUnitTestClassShouldHaveAsserts.xml

Co-authored-by: Andreas Dangel <andreas.dangel@adangel.org>
@tprouvot
Fix typo
50641a5
@tprouvot
PR review comment
2e9f25c
@tprouvot
Add new Assert methods to ApexAssertionsShouldIncludeMessageRule
c61108c
@tprouvot
Merge branch 'pmd:master' into feature/addNewClassForAssertion
d7cf959
@tprouvot
Merge branch 'feature/addNewClassForAssertion' of https://github.com/…
46aa491 
…tprouvot/pmd into feature/addNewClassForAssertion
@tprouvot
Add tests for new methods
d6194be
@tprouvot
Update add violation parameters to macth with new tests
ac94df9
@tprouvot
Remove param message not mandatory in this test
065b4b8
@tprouvot
Ad new tests for Assert method class
d73cdcc
@tprouvot
Copy link
Contributor Author

Thanks for the PR!

Please have a look at my comments. I think, we can edit #4096 to cover both rules and improve both rules with this PR.

Hi @adangel,
Thanks for your suggestions, the PR should contains the additional tests and corrections.

adangel
adangel approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@adangel adangel left a comment

Choose a reason for hiding this comment

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

Thanks!

.../net/sourceforge/pmd/lang/apex/rule/bestpractices/xml/ApexAssertionsShouldIncludeMessage.xml Show resolved Hide resolved
@adangel adangel added this to the 6.49.0 milestone Aug 29, 2022
adangel added a commit that referenced this pull request Aug 29, 2022
@adangel
[apex] Update tests (ApexUnitTestClassShouldHaveAsserts, ApexAssertio…
7c6277c 
…nsShouldIncludeMessage) #4097
adangel added a commit that referenced this pull request Aug 29, 2022
@adangel
[doc] Update release notes (#4097, #4096)
38e7e69
adangel added a commit that referenced this pull request Aug 29, 2022
@adangel
Merge pull request #4097 from tprouvot:feature/addNewClassForAssertion
54a2614 
[apex] ApexUnitTestClassShouldHaveAssertsRule: Support new Assert class (Apex v56.0) #4097
@adangel adangel merged commit 29c4e99 into pmd:master Aug 29, 2022
@tprouvot tprouvot deleted the feature/addNewClassForAssertion branch August 30, 2022 06:45
@adangel adangel mentioned this pull request Sep 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SCWells72 SCWells72 SCWells72 requested changes

@adangel adangel adangel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
6.49.0
4 participants
@tprouvot @SCWells72 @adangel @pmd-test