Conditional Compilation Fixes. #302
Conversation
Uncommenting buggy redirect tests. Adding xcconfig to put project settings into. Adding preprocessor macros to let unit test arguments through. Adding default value of preprocessor macros to default them to run conditional tests when using Xcode. Adding comments for xcconfig. Updating Rake arguments.
| // rake ios['iOS StaticLib','latest','build-for-testing test-without-building',"OHHTTPSTUBS_SKIP_REDIRECT_TESTS=1"] | ||
| OHHTTPSTUBS_SKIP_REDIRECT_TESTS=0 | ||
|
|
||
| GCC_PREPROCESSOR_DEFINITIONS=OHHTTPSTUBS_SKIP_TIMING_TESTS=$(OHHTTPSTUBS_SKIP_TIMING_TESTS) OHHTTPSTUBS_SKIP_REDIRECT_TESTS=$(OHHTTPSTUBS_SKIP_REDIRECT_TESTS) |
There was a problem hiding this comment.
Not sure that really matters in practice in our case but shouldn't we used $(inherited) in that definition? (I think the default in Xcode is GCC_PREPROCESSOR_DEFINITIONS=DEBUG=1 in debug — while it's empty in release — so not using $(inherited) here means that debug builds don't have DEBUG=1 set anymore? Not that we currently use #if DEBUG in OHHTTPStubs as far as I know but just for configuration consistency…
There was a problem hiding this comment.
I'm honestly not sure how, but the config file looks like it's value resolves to have DEBUG=1, even though it's in the project build settings and not in the file.
I thought build settings always inherited from right to left in Xcode's levels view (^^ that view).. so I wouldn't expect the config file to inherit a setting from the project, but it does. 🤷♂️
Now that I think more about it - I normally don't mix project build settings and also have an xcconfig with them. My opinion is normally that if you have an xcconfig you should put the entire value of that setting into the xcconfig instead of the project's build settings.
@AliSoftware - What do you think of moving DEBUG=1 to the xcconfig instead, so there is no project build setting for this one?
There was a problem hiding this comment.
Strange that it magically inherit project settings indeed 🧐
Moving DEBUG=1 in the xcconfig file makes sense, but would require two separate xcconfig files then (one for each config), just wondering if that's worth it 🤔
There was a problem hiding this comment.
Yes, probably overkill. So do you think we should add $(inherit) or leave it as is?
There was a problem hiding this comment.
Let's try $(inherited) at least for consistency, and validate it still works as expected (eg doesn't double-define DEBUG=1 in the resolved build settings after all)
There was a problem hiding this comment.
There were actually a few places where we had DEBUG=1 twice, so I removed the target level preprocessor macros where needed.
Here are the resulting resolved preprocessor macros now:
| NSLog(@"/!\\ Test skipped because the NSURLSession class is not available on this OS version. Run the tests a target with a more recent OS.\n"); | ||
| } | ||
| } | ||
| #endif | ||
| - (void)test_NSURLSessionEphemeralConfig |
There was a problem hiding this comment.
(Just because I like nitpicking: this feels a bit packed, so if you have the occasion of a new commit in this PR, adding an extra empty line to separate the endif and the next function while you're at it might help us breathe 😅)
There was a problem hiding this comment.
Addressed in 6bb7397 .
| GCC_PREPROCESSOR_DEFINITIONS = ( | ||
| "DEBUG=1", | ||
| "$(inherited)", | ||
| ); |
There was a problem hiding this comment.
These were added at the target level, and caused DEBUG=1 to end up in the resolved macros twice.
| GCC_PREPROCESSOR_DEFINITIONS = ( | ||
| "DEBUG=1", | ||
| "$(inherited)", | ||
| ); |
There was a problem hiding this comment.
These were added at the target level, and caused DEBUG=1 to end up in the resolved macros twice.
| GCC_PREPROCESSOR_DEFINITIONS = ( | ||
| "DEBUG=1", | ||
| "$(inherited)", | ||
| ); |
There was a problem hiding this comment.
These were added at the target level, and caused DEBUG=1 to end up in the resolved macros twice.
| GCC_PREPROCESSOR_DEFINITIONS = ( | ||
| "DEBUG=1", | ||
| "$(inherited)", | ||
| ); |
There was a problem hiding this comment.
These were added at the target level, and caused DEBUG=1 to end up in the resolved macros twice.
| GCC_PREPROCESSOR_DEFINITIONS = ( | ||
| "DEBUG=1", | ||
| "$(inherited)", | ||
| ); |
There was a problem hiding this comment.
These were added at the target level, and caused DEBUG=1 to end up in the resolved macros twice.
| - osx_image: xcode8.3 | ||
| env: RAKETASK="osx[Mac Framework,x86_64,build test,-DOHHTTPSTUBS_SKIP_TIMING_TESTS=1]" | ||
| env: RAKETASK="osx[Mac Framework,x86_64,build test,OHHTTPSTUBS_SKIP_TIMING_TESTS=1 OHHTTPSTUBS_SKIP_REDIRECT_TESTS=1]" |
There was a problem hiding this comment.
I'm happy surprised that the space character added in there doesn't break anything btw 😅 I guess I properly remembered in the CI config file to use quotes around "$RAKETASK" when incoming take, pfew 😄
|
Man it's so refreshing to see CI turn green on the PR without having to restart every CI job manually and pray until it passes 😄🎉 |
Uncommenting buggy redirect tests.
Adding xcconfig to put project settings into.
Adding preprocessor macros to let unit test arguments through.
Adding default value of preprocessor macros to default them to run conditional tests when using Xcode.
Adding comments for xcconfig.
Updating Rake arguments.
Checklist
Description
My original goal was to add a previously commented unit test back to the test suite, and to enable it when testing in Xcode but disable it when testing in CI. During this investigation, I also noticed that the Timing Tests were not working in this way like they were originally intended to. I ended up fixing those tests in this PR too.
Motivation and Context
Timing Tests -
If you look at previous test runs, you see that timing tests were being run in Travis:
Even though the right Xcode argument was being passed:
This seems like a bug with the project setup.
I found that we needed to add a preprocessor macro to the project's build settings to let the
xcodebuildargument through.I always like using
xcconfigfiles to manage build settings, so I created a project levelxcconfigwith the build settings and commented the config. Build settings that are defined inxcconfigfiles can always be overridden by passing inxcodebuildarguments.Redirect Tests -
I followed the same pattern with the uncommented redirect tests.
End Result -
You can see here that both sets of tests are run when using Xcode:
And that neither are ran during CI: