Skip to content

Assert tests with localized values #21982

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Assert tests with localized values #21982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion WordPress/WordPressTest/Dashboard/BlazeCampaignViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ final class BlazeCampaignViewModelTests: XCTestCase {
XCTAssertEqual(viewModel.imageURL, URL(string: "https://i0.wp.com/public-api.wordpress.com/wpcom/v2/wordads/dsp/api/v1/dsp/creatives/56259/image?w=600&zoom=2"))

// Then stats are displayed
XCTAssertEqual(viewModel.impressions, "1,000")
XCTAssertEqual(viewModel.impressions, 1_000.abbreviatedString())
Copy link
Contributor

@mokagio mokagio Nov 8, 2023

Choose a reason for hiding this comment

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

Thanks for looking into this @dvdchr.

By calling abbreviatedString() here, it seems like we are testing more at the implementation level than the behavior level. That is, reading the test one doesn't learn what the expected impressions value, only that it should be whatever abbreviatedString() returns.

For reference, here's the production code, which, unsurprisingly, calls abbreviatedString():

WordPress-iOS/WordPress/Classes/ViewRelated/Blog/Blog Dashboard/Cards/Blaze/DashboardBlazeCampaignView.swift

Line 144 in 697186e

self.impressions = campaign.stats?.impressionsTotal?.abbreviatedString() ?? "0"

This is probably okay practically, because we are not likely are we to change this formatting...

Still, I think we would be better served by making the application language and region deterministic, as you suggested in the PR description.

Copy link
Contributor

@hassaanelgarem hassaanelgarem Nov 8, 2023

Choose a reason for hiding this comment

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

Still, I think we would be better served by making the application language and region deterministic, as you suggested in the PR description.

Plus 1 on this. While this fixes tests for Indonesian, I tried running DashboardStatsViewModelTests with Arabic as the locale, and it still failed.

XCTAssertEqual(viewModel.clicks, "235")
XCTAssertTrue(viewModel.isShowingStats)
}
Expand Down
6 changes: 3 additions & 3 deletions WordPress/WordPressTest/Dashboard/DashboardStatsViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@ class DashboardStatsViewModelTests: XCTestCase {
let viewModel = DashboardStatsViewModel(apiResponse: apiResponse)

// When & Then
XCTAssertEqual(viewModel.todaysViews, "10,000")
XCTAssertEqual(viewModel.todaysVisitors, "200.0K")
XCTAssertEqual(viewModel.todaysLikes, "3.0M")
XCTAssertEqual(viewModel.todaysViews, 10_000.abbreviatedString(forHeroNumber: true))
XCTAssertEqual(viewModel.todaysVisitors, 200_000.abbreviatedString(forHeroNumber: true))
XCTAssertEqual(viewModel.todaysLikes, 3_000_000.abbreviatedString(forHeroNumber: true))
}

func testReturnZeroIfAPIResponseIsEmpty() {
Expand Down