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

changes to allow Supplementer to be called from ApplicationSObjectDo… #35

Closed
wants to merge 2 commits into from

Conversation

dmchaplin
Copy link

@dmchaplin dmchaplin commented May 5, 2020

…main.

proposed solution:
Fixes #33

supplement would no longer need to be called explicitly during tests.


This change is Reviewable

@stohn777
Copy link
Collaborator

stohn777 commented May 7, 2020

Hi @dmchaplin.

Thank you for your interest in and contributing to the AT4DX library.

The TestDataSupplementer is an anti-pattern to proper, preferred, mock-based testing, although many codebases have legacy, DML-based unit tests that can be undermined by changes to record-validity logic, e.g. modifying a SObject to have a required field which was previously optional. Any DML-based test that didn't address the field in the past, will be broken at this point. The test-data supplementation scheme addresses this problem without having to potentially embark on an inconvenient, extensive effort of replacing all broken logic with mock testing. It's a convenient workaround for the time when legacy patterns have yet to be transitioned.

With the sole purpose of this pattern being temporary and testing based, including it in the ApplicationSObjectDomain class violates separation of concerns principles.

The supplementer supports two SOLID architecture principles:

  • Encapsulation of the record-validation logic in the proper scope. Account record-validation logic can be contained in the same scope as the Account yet accessible to dependent scopes needing it.
  • Dependency-injection-based logic for extending related logic. Once implemented in a dependent scope, extensions to the record-validation rules will automatically be picked up.

The AT4DX sample code repo [1] has examples of the intended usage of the pattern. Note the two files.

  • sfdx-source/reference-implementation-marketing/main/classes/supplementers/MarketingFieldsForAccountsSupplementer.class
  • sfdx-source/reference-implementation-marketing/test/classes/AccountSupplementationTest.class

MarketingFieldsForAccountsSupplementer would exist within the same scope as the Account SObject, and AccountSupplementationTest exemplifies the unit-test usage of the TestSupplementer to further populate an Account with record-validation-related data.

[1] https://github.com/ImJohnMDaniel/at4dx-samplecode

@dmchaplin
Copy link
Author

@stohn777

Thanks for the well-thought-out response. Learning AT4DX has been very rewarding.

I've never liked using .isRunningTest() in code, but SOC is a good justification for why that is the case.

It makes sense that MarketingFieldsForAccountsSupplementer would exist in the same scope as the Account Sobject. How does the legacy code know to call supplement()? Is it a one-time edit to each of those test classes to add it?

Thanks, shiny side up and tailwinds.
Drew

@stohn777
Copy link
Collaborator

stohn777 commented May 7, 2020

@dmchaplin

You're correct that the unit tests would need the one-time addition of the TestSupplementer logic, as conceptualized in the AccountSupplementationTest.cls.

"shiny side up and tailwinds" Are you a fan of aviation? 💯

Best wishes.

@dmchaplin
Copy link
Author

dmchaplin commented May 11, 2020

@stohn777

Yes, I own a '67 Skyhawk and have somewhere around 600 hours. Somewhere along the line, I spotted your linked-in profile and thought I'd give you a shout-out.

Copy link
Collaborator

@stohn777 stohn777 left a comment

Choose a reason for hiding this comment

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

@dmchaplin
Partly as an experiment since we recently implemented Github Actions to automatically run unit tests on PRs, would you be so kind as to submit a white-space change to kick-off that process?

I looked for a way to do so myself but was unsuccessful. I will completely understand if you're too busy to do so.

Thanks!

White-space change to initiate unit-test run.
@stohn777
Copy link
Collaborator

@dmchaplin
I got it. Never mind. :-)

@stohn777
Copy link
Collaborator

@dmchaplin This change was incorporated into the recent "improved test coverage" work. Thank you for the great catch and solution on this. GG

1341480

@stohn777 stohn777 closed this Mar 31, 2021
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.

calling TestDataSupplement
2 participants