[cmpcodesize][1] Extract otool subprocess calls #636
Closed
Conversation
This file contains 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
The functions in `cmpcodesize.compare` do several things: they call otool, they use regex matchers to extract information from otool's output, and they output that information using `print()`. Currently, the only way to test cmpcodesize is via functional tests: ones that actually run on a dylib like libswiftCore.dylib. This takes a lot of time and is difficult to fully automate. By extracting otool calls from `cmpcodesize.compare`, we can test those in isolation. Furthermore, future commits can test the functions in `cmpcodesize.compare` using canned strings, instead of actual otool output.
modocache
changed the title
|
This should probably be reviewed by @eeckstein , @swiftix, and @nadavrot. 👋 |
|
@modocache Should we take another look at those change - do you have time to resolve the conflicts? :-) |
|
I'd like to confirm this script is used at all first. If no one uses it I think it'd be better to delete than to maintain. @eeckstein, are you aware of anyone using this? |
|
The script is used a lot. |
|
Great! Thanks! |
|
Sorry for letting this languish. I don't think I have the capacity to shepherd this at the moment. I'll close this pull request to reduce the amount of noise on the repository. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What's in this pull request?
The functions in
cmpcodesize.comparedo several things: they call otool, they use regex matchers to extract information from otool's output, and they output that information usingprint().Currently, the only way to test cmpcodesize is via functional tests: ones that actually run on a dylib like libswiftCore.dylib. This takes a lot of time and is difficult to fully automate.
The commit in this pull request extracts otool calls from
cmpcodesize.compare. It also adds tests for those calls.Future commits can test the functions in
cmpcodesize.compareusing canned strings, instead of actual otool output.Why merge this pull request?
cmpcodesize.comparemore modular, and eliminates all uses of theflatten()function.cmpcodesize. Instead of running functional tests* on a sample dylib, we can refactor it such that we passcmpcodesizesample strings, and verify its behavior parsing those sample strings.What downsides are there to merging this pull request?
I suppose some could argue the tests in
utils/cmpcodesize/tests/test_otool.pydon't provide much value--they're testing some very simple logic.