-
Notifications
You must be signed in to change notification settings - Fork 518
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
Feature/unit test without dml #223
Feature/unit test without dml #223
Conversation
@wimvelzeboer would be nice to have org independent test classes to have a generic framework. Could you elaborate on which lines of code you specifically changed? For now I could only see syntax/spacing changes. Thanks! |
Yes agree with @foxcreation kinda hard to see the wood for the trees. Also i would maybe focus on just the changes for now. There is an outstanding issue to add PMD to this repo so we will have to agree what the formatting rules are and likely make changes anyway. Can I thus propose that you update this PR with just the key changes you need and thus for now its easier to review? |
163704d
to
482c2c0
Compare
@foxcreation & @afawcett I removed a lot of the reformatting so that the actual changes are more clear. Hope this helps with the review. |
bd53fad
to
c4d2ca1
Compare
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.
@wimvelzeboer nice changes and great to get rid of all inserts. Maybe we can move the test class to be inParallel as well?
@istest( isParallel = true )
@foxcreation Good idea! I have added it! |
7fca073
to
4062861
Compare
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.
Very nice work!
@wimvelzeboer thank you! |
0da9492
4062861
to
0da9492
Compare
@afawcett @foxcreation @ImJohnMDaniel Please review again. I've rebased the branch again to resolve the merge conflict |
@wimvelzeboer apologies for the delay - looks good - added my review. cc @foxcreation @ImJohnMDaniel |
DML statement were removed from unit test to avoid failing tests
ad8ad3a
0da9492
to
ad8ad3a
Compare
@afawcett @foxysolutions @ImJohnMDaniel |
@afawcett @daveespo @ImJohnMDaniel I know that there is work going to be done on the QueryFactory in the Selector, but could we merge this PR in the mean time? |
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.
Reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @afawcett and @daveespo)
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.
Reviewed 1 of 1 files at r1.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @afawcett and @daveespo)
The Unit tests are failing from this fflib_QueryFactoryTest class on same Orgs. Especially when there are mandatory fields on the account, contact or task object.
Also did a bit of reformatting of the source code.
This change is