Skip to content

Conversation

@mokagio
Copy link
Contributor

@mokagio mokagio commented Jul 19, 2021

Includes #16877 by @emilylaguna.

Notice that I switched to a branch in the release toolkit and then reverted just so I could get the fix submitted quickly. Normally, I would have wanted to ship a new version of the toolkit and switch to it, but this is an hotfix so I prioritized speed.

There were conflicts on:

  • WordPress/WordPressTest/BlogTests.swift
  • config/Version.internal.xcconfig
  • config/Version.public.xcconfig

Both this branch and the hotfix added tests to BlogTests.swift, so I kept them both, and verified they all pass still.

I discarded the hotfix version changes because 17.8 is the latest version number.

Emily Laguna and others added 12 commits July 15, 2021 23:03
…on-crash

Fixes a crash when the value returned by Blog.version is not a string
Merge branch 'release/17.7.1' into merge/hotfix-17.7.1-into-release-17.8

Conflicts:

- `WordPress/WordPressTest/BlogTests.swift`
- `config/Version.internal.xcconfig`
- `config/Version.public.xcconfig`

Both this branch and the hotfix added tests to `BlogTests.swift`, so I
kept them both, and verified they all pass still.

I discarded the hotfix version changes because 17.8 is the latest
version number.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 19, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jul 19, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@mokagio mokagio requested a review from a team July 19, 2021 06:02
@mokagio mokagio added this to the 17.8 ❄️ milestone Jul 19, 2021
@mokagio
Copy link
Contributor Author

mokagio commented Jul 19, 2021

The unit tests are failing on this branch, but were good on the release branch after the Guntenberg update PR.

😞

I tried to understand what's going on, but I only got as far as seeing that in fetchTheme(_ completion:) in BlockEditorSettingsService, the checksums for both the new and old settings are "" and the theme results as not loaded.

Here's some pings to people who might know more:

  • @emilylaguna you made the hotfix change. I added some breakpoints and saw Blog version is called.
  • @chipsnyder I think you're the author of the logic and tests that are failing, maybe you have some idea?
  • @AmandaRiu @ceyhun you folks have worked on the latest Gutenberg changes and the failing tests are around the block editor

I'm sure between the 5 of us, we'll figure out what's going on 😄


Tip: the tests failed locally for me with a crash due to a force unwrap. To get more information out of the tests, I modified them like this:

diff --git a/WordPress/WordPressTest/BlockEditorSettingsServiceTests.swift b/WordPress/WordPressTest/BlockEditorSettingsServiceTests.swift
index b1a1f469af..b02f699d46 100644
--- a/WordPress/WordPressTest/BlockEditorSettingsServiceTests.swift
+++ b/WordPress/WordPressTest/BlockEditorSettingsServiceTests.swift
@@ -202,8 +202,17 @@ class BlockEditorSettingsServiceTests: XCTestCase {
         } else {
             XCTAssertNil(self.blog.blockEditorSettings?.rawStyles)
         }
-        XCTAssertGreaterThan(self.blog.blockEditorSettings!.colors!.count, 0)
-        XCTAssertGreaterThan(self.blog.blockEditorSettings!.gradients!.count, 0)
+
+        // This is to avoid the test crashing if when trying to access properties of a nil
+        // `blockEditorSettings` value later on. When the tests crash, we loose information on the
+        // assertions that run before the crash, which is a shame: the more feedback we have from
+        // the tests, the easier our triaging job will be.
+        guard let settings = self.blog.blockEditorSettings else {
+            return XCTFail("Blog blockEditorSettings are nil")
+        }
+
+        XCTAssertGreaterThan(settings.colors!.count, 0)
+        XCTAssertGreaterThan(settings.gradients!.count, 0)
     }
 }

The change above is far from ideal, but does the job of avoiding the crash. A better long term solution would be to rework that test helper to support throws and use XCTUnwrap. Another option could be to import Nimble and use its expectation for greater than, which can safely handle Optional.

@chipsnyder
Copy link
Contributor

Thanks for the ping @mokagio looking into this now.

@chipsnyder
Copy link
Contributor

@mokagio Here are the changes that should resolve this issue #16888

The change above is far from ideal, but does the job of avoiding the crash. A better long term solution would be to rework that test helper to support throws and use XCTUnwrap. Another option could be to import Nimble and use its expectation for greater than, which can safely handle Optional.

Good suggestions I took the spirit of this to capture what the error is in the PR I opened.

@mokagio
Copy link
Contributor Author

mokagio commented Jul 20, 2021

I merged @chipsnyder's PR (thank you Chip! 🙌) if the tests pass now we should be good, but can I get another explicit approval @oguzkocer / @wordpress-mobile/owl-team? Thanks 🙇

@mokagio mokagio marked this pull request as ready for review July 20, 2021 06:44
@mokagio mokagio requested a review from oguzkocer July 20, 2021 06:44
@peril-wordpress-mobile
Copy link

Messages
📖 This PR has the 'Releases' label: some checks will be skipped.

Generated by 🚫 dangerJS

@mokagio mokagio requested a review from a team July 20, 2021 06:44
@mokagio mokagio merged commit 0a5c608 into release/17.8 Jul 20, 2021
@mokagio mokagio deleted the merge/hotfix-17.7.1-into-release-17.8 branch July 20, 2021 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants