Skip to content

Conversation

@owenv
Copy link
Collaborator

@owenv owenv commented Feb 5, 2025

These asserts never worked and aren't useful, but we're inactive until recently due to an erroneous nil check. Drop them from the test entirely

@owenv
Copy link
Collaborator Author

owenv commented Feb 5, 2025

@swift-ci test

@mhrawdon
Copy link
Contributor

mhrawdon commented Feb 5, 2025

I think the intent of these assertions was to check that the build phases were being loaded as the correct types, and that they contained the correct build files. Could they be re-worked to check that more reliably? (They way they're written now is certainly pretty clunky, and the force-unwraps likely just unnecessary given how tests work today.) Or are the PIFLoadingTests generally not very useful at this point?

@owenv
Copy link
Collaborator Author

owenv commented Feb 5, 2025

I couldn't figure out the intent here, because we're comparing BuildFiles to FileReferences, which doesn't seem like it should ever work

@jakepetroules
Copy link
Collaborator

Yeah, this definitely seems like it was always broken.

@mhrawdon
Copy link
Contributor

mhrawdon commented Feb 5, 2025

After discussing with Owen, I'm going to take a crack at resolving this in a different way. I may do a pass through this file (which is one of the oldest files in the code base) to make it more robust (e.g., to eliminate force-unwraps).

@mhrawdon
Copy link
Contributor

mhrawdon commented Feb 5, 2025

I think this PR should fix these issues (and clean up a bunch of code besides).

@owenv owenv closed this Feb 6, 2025
@jakepetroules jakepetroules deleted the owenv-drop-broken-asserts branch April 26, 2025 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants