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

Needed Code Coverage > 75% #63

Merged
merged 10 commits into from
Feb 1, 2021

Conversation

bjanderson70
Copy link
Contributor

@bjanderson70 bjanderson70 commented Jul 13, 2020

  • Most of the changes were to up the code coverage > 75%. Customer will not accept otherwise;
  • Mark some methods TestVisible and made some defined bindings (in tests) visible to other Tests
  • Other Changes:
    ===========
    • platform cache needed to check the partition was present
      ++ otherwise, throws an exception
      ++ false assumption the partition is present, if enabled in metadata
    • updated the Scratch-Org definition (avoid warnings)

This change is Reviewable

@afawcett
Copy link
Collaborator

@bjanderson70 this is great work! Thank you sooo much! Looks great to me. But lets have @ImJohnMDaniel review as he added the platform cache feature as well.

afawcett
afawcett previously approved these changes Jul 20, 2020
@ImJohnMDaniel ImJohnMDaniel self-requested a review July 20, 2020 18:29
… into injected components; ie. my_comp.find("my_item").find("di-component").callMyMethod()
deploy from this repo ... until changes (pull requests) are accepted.
Copy link
Contributor

@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.

Overall great appreciation is extended for addressing. I know that other AEP projects could use test coverage help.

Good job and thank you!!!

Only show-stoppers for me are the questions concerning:

  • Suggestion to use third-party plugins for installation in README.md.
  • di_PlatformCache.cls logging functionality.

I did notice that in some classes some effort was put forth in cleaning up "pretty code" matters, but in the new test classes, some "pretty code" inconsistencies exist -- dangling new lines, unnecessary comments, etc. Although not a show stopper, would it be within your capacity to tidy-up those inconsistencies?

README.md Outdated Show resolved Hide resolved
force-di/main/classes/di_Binding.cls Outdated Show resolved Hide resolved
force-di/main/classes/di_BindingParam.cls Outdated Show resolved Hide resolved
force-di/main/classes/di_Injector.cls Show resolved Hide resolved
force-di/main/classes/di_PlatformCache.cls Show resolved Hide resolved
force-di/main/classes/di_PlatformCache.cls Outdated Show resolved Hide resolved
force-di/main/classes/di_PlatformCache.cls Show resolved Hide resolved
…eKeyIndexMap" as I was not certain if originator had a purpose with that code
@afawcett
Copy link
Collaborator

I just took a deeper look at this - agree with sentiment around not including the scratch org config nor the custom CLI plugin reference since its more of personal preference thing. Overall the code coverage part of this is good. However this PR does also include an enhancement to the way the aura/lwc components monitor for attribute changes (vs just setup on init). Again this is good - and on this occasion to move this forward fine to be part of the PR IMHO - but in future its best to submit one feature / enhancement per PR. @ImJohnMDaniel is going to take a further look later today. Thank you for the submission.

ImJohnMDaniel
ImJohnMDaniel previously approved these changes Feb 1, 2021
@ImJohnMDaniel ImJohnMDaniel merged commit 6e11ae2 into apex-enterprise-patterns:master Feb 1, 2021
@ImJohnMDaniel
Copy link
Collaborator

@bjanderson70 -- Thanks again for all of this work. It is very much appreciated.

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.

5 participants