-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix language switching and update date-fns to v4 #2788
Changes from all commits
56918c5
6ad1264
c93a7be
3d0d05a
9cacb26
0beff0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,14 +27,14 @@ const episode = EpisodeModel.create(data) | |
test("publish date format", () => { | ||
expect(episode.datePublished.textLabel).toBe("Jan 20, 2022") | ||
expect(episode.datePublished.accessibilityLabel).toBe( | ||
'demoPodcastListScreen:accessibility.publishLabel {"date":"Jan 20, 2022"}', | ||
"demoPodcastListScreen:accessibility.publishLabel", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain please why are we removing the date? Reading from the PR description, it seems like we still want to improve this test later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really understand why this changed, but I also didn't want to block this on this test, because the behavior is correct when I actually test it in the app. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The single quotes changing to double quotes was required by the linter, and I think might be related to tests having a separate TS config? but not sure, I thought that would be handled by prettier. Actually I'm even more confused by this because apparently it failed on the CLI project's prettier check (https://app.circleci.com/pipelines/github/infinitered/ignite/3631/workflows/6e7e16e3-e4c1-4a6b-949b-d1fe26da1665/jobs/4375), but the inner project isn't complaining about the double-quotes. That config just isn't unified, so we get weird things like this sometimes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My question was not related to quotes but related to deleting the date within it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, for that part see my first comment. I'm not sure why it changed, but I'd also like to not block this PR on that since the logic is working correctly on the real app, this is just a test that I'm not sure is testing anything useful the way it's currently set up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed issue for this: #2789 |
||
) | ||
}) | ||
|
||
test("duration format", () => { | ||
expect(episode.duration.textLabel).toBe("42:58") | ||
expect(episode.duration.accessibilityLabel).toBe( | ||
'demoPodcastListScreen:accessibility.durationLabel {"hours":0,"minutes":42,"seconds":58}', | ||
"demoPodcastListScreen:accessibility.durationLabel", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as here. |
||
) | ||
}) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it be worth writing a test for this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking splitting on
-
and checkingincludes
were simple enough it didn't need a test, if we did something more complex maybe then?Most of the complexity here comes in with third party systems, so an E2E test to check that we can switch languages makes the most sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree this is not complex, but I'm also thinking of future Felipe doing an i18 migration again. It's all good; not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure. I still think in that case what is most useful is an E2E test instead of unit tests.