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.

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

Jump to bottom

[Test Fix] Fix test fail on non-English locale (SR-15345) #7

Merged
merged 2 commits into from
Oct 27, 2021
Merged

[Test Fix] Fix test fail on non-English locale (SR-15345) #7

merged 2 commits into from
Oct 27, 2021

Conversation

Kyle-Ye
Copy link
Collaborator

@Kyle-Ye Kyle-Ye commented Oct 16, 2021

Bug
https://bugs.swift.org/browse/SR-15345

Summary

Fix test fail on non-English locale machine

Testing

  1. Change the machine locale to a non-English locale
  2. Run the ./bin/test

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran the ./bin/test script and it succeeded

Kyle-Ye
Kyle-Ye commented Oct 16, 2021
View reviewed changes
Tests/SwiftDocCTests/Model/SemaToRenderNodeTests.swift Outdated
@@ -639,13 +639,13 @@ class SemaToRenderNodeTests: XCTestCase {
}
XCTAssertEqual(firstTutorialReference.url, "/tutorials/test-bundle/testtutorial")
XCTAssertFalse(firstTutorialReference.abstract.isEmpty)
XCTAssertEqual(firstTutorialReference.estimatedTime, "20min")
XCTAssertEqual(firstTutorialReference.estimatedTime, translator.contentRenderer.formatEstimatedDuration(minutes: 20))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the purpose of the test expression?

I just afraid of that if the purpose is to test DocumentationContentRenderer'formatEstimatedDuration(minutes:), it seemed the test now is useless, since we call the function again on the expect string.

And what is the best solution for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, part of the purpose here is to test this behavior. I’m assuming you’re seeing a test failure on macOS because DateComponentsFormatter (correctly) localizes the date. Since general support for localization is tracked by SR-15352, I’d suggest we instead use the Linux behavior on all platforms and re-introduce using DateComponentsFormatter as part of that work.

@franklinsch franklinsch self-requested a review October 18, 2021 12:02
franklinsch
franklinsch reviewed Oct 18, 2021
View reviewed changes
Copy link
Member

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Thanks for opening this, Kyle!

Tests/SwiftDocCTests/Model/SemaToRenderNodeTests.swift Outdated
@@ -639,13 +639,13 @@ class SemaToRenderNodeTests: XCTestCase {
}
XCTAssertEqual(firstTutorialReference.url, "/tutorials/test-bundle/testtutorial")
XCTAssertFalse(firstTutorialReference.abstract.isEmpty)
XCTAssertEqual(firstTutorialReference.estimatedTime, "20min")
XCTAssertEqual(firstTutorialReference.estimatedTime, translator.contentRenderer.formatEstimatedDuration(minutes: 20))
Copy link
Member

Choose a reason for hiding this comment

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

Yes, part of the purpose here is to test this behavior. I’m assuming you’re seeing a test failure on macOS because DateComponentsFormatter (correctly) localizes the date. Since general support for localization is tracked by SR-15352, I’d suggest we instead use the Linux behavior on all platforms and re-introduce using DateComponentsFormatter as part of that work.

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 20, 2021

If I understand you correctly, the suggestion is to pending the PR and the the bug until the Linux implementation of DateComponentsFormatter, right?
And the workaround it to set the test machine's locale to English.

@franklinsch
Copy link
Member

franklinsch commented Oct 21, 2021 •
edited
Loading

I think we should use your PR here to remove the use of DateComponentsFormatter and expand the implementation that’s currently for Linux to all platforms until we have full support for localized documentation content:

let hours = minutes / 60
let minutes = minutes % 60
return "\(hours > 0 ? "\(hours)hr " : "")\(minutes)min"

Then as part of SR-15352, we’ll want to add DateComponentsFormatter back, since that will bring localization support in general. Does that make sense?

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 22, 2021

I think we should use your PR here to remove the use of DateComponentsFormatter and expand the implementation that’s currently for Linux to all platforms until we have full support for localized documentation content:

let hours = minutes / 60
let minutes = minutes % 60
return "\(hours > 0 ? "\(hours)hr " : "")\(minutes)min"

Then as part of SR-15352, we’ll want to add DateComponentsFormatter back, since that will bring localization support in general. Does that make sense?

I agree. Updated the PR

@Kyle-Ye @franklinsch
Update Sources/SwiftDocC/Model/Rendering/DocumentationContentRenderer…
901ccb4 
….swift

Co-authored-by: Franklin Schrans <f.schrans@me.com>
franklinsch
franklinsch approved these changes Oct 22, 2021
View reviewed changes
Copy link
Member

@franklinsch franklinsch left a comment

Choose a reason for hiding this comment

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

Thank you @Kyle-Ye for bringing this up and iterating during the review, this is looking great! I will merge this once Linux CI is up and running again (i.e., when SR-15362 lands in a nightly).

@Kyle-Ye
Copy link
Collaborator Author

Kyle-Ye commented Oct 27, 2021

Thank you @Kyle-Ye for bringing this up and iterating during the review, this is looking great! I will merge this once Linux CI is up and running again (i.e., when SR-15362 lands in a nightly).

The new toolchain is landed, could you please make the ci-bots to test on this?

@franklinsch
Copy link
Member

@swift-ci please test

@franklinsch
Copy link
Member

@swift-ci please test Linux

@franklinsch franklinsch merged commit 2cdc1e4 into apple:main Oct 27, 2021
@franklinsch
Copy link
Member

Thanks @Kyle-Ye !

@Kyle-Ye Kyle-Ye deleted the bugfix/localization_time branch October 27, 2021 11:35
This was referenced Oct 21, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franklinsch franklinsch franklinsch approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Kyle-Ye @franklinsch