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

[No Ticket] Full MockOptimizely Removal PR Fixes/Cleanup #1825

Merged
merged 8 commits into from Jun 12, 2023

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Jun 8, 2023

📲 What

Regarding my Full MockOptimizely Removal PR
There are some further changes and cleanup that can be done.

🤔 Why

Its important to be thorough in code cleanup, making sure to really understand why certain changes are being made and how best to address removing and/or cleaning up so that we can guarantee that our app is better for it.

🛠 How

  • Adding tests around our .ksr_remoteConfigClientConfigured & .ksr_remoteConfigClientConfigurationFailed notifications
  • Simplifying properties like paymentSheetEnabled & campaignTabEnabled that were previously updated
  • Removing redundant tests
  • Removing Snapshots associated with snapshot tests that were removed and making sure only affected snapshots are updated

✅ Acceptance criteria

  • all tests pass

⏰ TODO

  • Discuss snapshot changes from old PR with Mubarak to get better aligned on why they were so many changes in the first place and how best to approach next steps.

@scottkicks scottkicks self-assigned this Jun 8, 2023
@scottkicks scottkicks added this to the release-5.9.0 milestone Jun 8, 2023
@codecov
Copy link

codecov bot commented Jun 8, 2023

Codecov Report

Merging #1825 (8e9db64) into main (0e92dd6) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1825   +/-   ##
=======================================
  Coverage   84.35%   84.36%           
=======================================
  Files        1276     1276           
  Lines      115449   115436   -13     
  Branches    30743    30738    -5     
=======================================
- Hits        97392    97383    -9     
+ Misses      16990    16986    -4     
  Partials     1067     1067           
Impacted Files Coverage Δ
...ents/Controllers/CommentsViewControllerTests.swift 100.00% <ø> (ø)
Kickstarter-iOS/AppDelegateViewModelTests.swift 98.67% <100.00%> (+0.01%) ⬆️
Library/ViewModels/PaymentMethodsViewModel.swift 98.24% <100.00%> (-0.03%) ⬇️
...ary/ViewModels/PledgePaymentMethodsViewModel.swift 92.58% <100.00%> (-0.04%) ⬇️
...iewModels/PledgePaymentMethodsViewModelTests.swift 97.57% <100.00%> (ø)
.../ViewModels/ProjectPamphletMainCellViewModel.swift 95.87% <100.00%> (-0.04%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@scottkicks scottkicks marked this pull request as ready for review June 8, 2023 19:39
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

So close to perfect, haha, just remember if we remove a test in a view controller to remove the corresponding screenshots.

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

[Assuming tests pass] Update the milestone to 5.9.0 - but don't merge this pr yet. Let's merge the marketing version update for 5.8.0 first, close out milestone 5.8.0 (no action required), update the release branch (main --> release-5.8.0 PR), then merge this in.

@scottkicks scottkicks merged commit 7015f3b into main Jun 12, 2023
7 checks passed
@scottkicks scottkicks deleted the scott/optimizely-removal-fixes branch June 12, 2023 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants