Fix PIFLoadingUnitTests.loadingStandardTarget() and clean up other tests in that suite. #100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhrawdon
requested review from
jakepetroules,
mirza-garibovic,
neonichu and
owenv
as code owners
Contributor
Author
|
@swift-ci test |
owenv
approved these changes
Feb 6, 2025
neonichu
approved these changes
Feb 6, 2025
jakepetroules
approved these changes
Feb 6, 2025
plemarquand
reviewed
Feb 6, 2025
…sts in that suite. loadingStandardTarget() had several checks which were failing, and had been broken for a long time but had been masked by other behavior which was recently fixed. Many other tests were written early in the project's history, and could be updated to use newer paradigms. In particular, many unnecessary optionals and force-unwraps have been removed or converted. One of the principles applied here is that tests should strive to perform as much as their testing as is reasonable, and not simply stop on first failure. Even lines which fail an #expect() can often move on to the section block of the test rather than skipping everything afterwards.
mhrawdon
force-pushed
the
mhrawdon/PIFLoadingUnitTests-fixes-and-cleanup
branch
from
73c2954 to
ad0d3bf
Compare
Contributor
Author
|
@swift-ci test |
| do | ||
| { | ||
| let fileRef = fileGroup.children[0] as! FileReference | ||
| if let fileRef = try? #require(fileGroup.children.first as? FileReference) { |
Collaborator
There was a problem hiding this comment.
Unfortunately this pattern introduced some warnings and also unintentionally changed behavior -- since there is no else case, a previous crash is now an unexpected pass rather than a failure. Fixing in #135
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
loadingStandardTarget()had several checks which were failing, and had been broken for a long time but had been masked by other behavior which was recently fixed.Many other tests were written early in the project's history, and could be updated to use newer paradigms. In particular, many unnecessary optionals and force-unwraps have been removed or converted.
One of the principles applied here is that tests should strive to perform as much as their testing as is reasonable, and not simply stop on first failure. Even lines which fail an
#expect()can often move on to the section block of the test rather than skipping everything afterwards.