Skip to content

DarwinComptibilityTests: Add Xcode Project #1286

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
Merged
merged 3 commits into from
Nov 28, 2017
Merged

DarwinComptibilityTests: Add Xcode Project #1286

merged 3 commits into from
Nov 28, 2017

Conversation

spevans
Copy link
Contributor

@spevans spevans commented Oct 27, 2017 •
edited
Loading

This PR adds a new Xcode Project DarwinCompatibiltyTests that compiles the current swift-corelibs-foundation tests against the native Foundation and XCTest. This allows tests to be validated against the native Foundation and is useful for checking edge cases especially for types such as NSNumber, Decimal etc. The tests show up as unit tests in Xcode so can be run individually.

All of the tests currently compile, with the help of some compatibility shims, however some of the tests fail. Tests that hang or crash are commented out using the #if !DARWIN_COMPATIBILTY_TESTS/#endif pair which is defined in the project's compiler flags.

On 10.12 about 100 tests (185 individual XCTAsserts) currently fail for various reasons. Also some methods require 10.13 etc (currently using if #available() but this isn't a permanent solution).

The xdgTestHelper is also not currently compiled and is disabled in the tests as I could not find the cookie storage location, but this can be fixed in the future.

This has been tested on 10.12 (Sierra) but not 10.13 (High Sierra). I won't fix all of the tests before this is merged as there will be too many to fix upfront and some may also pass on 10.13 but not 10.12, however it is envisaged that tests will be fixed on an ongoing basis.

Any comments, suggestions welcome.

  • Add Xcode project DarwinCompatibilityTests
  • Add shims and minor test fixes
  • Comment out tests that don't compile/run against native Foundation using DARWIN_COMPATIBILITY_TESTS flag.

@spevans spevans force-pushed the pr_darwin_compatibility_tests branch from 576bad3 to 20035f9 Compare October 30, 2017 06:43
@parkera
Copy link
Contributor

parkera commented Oct 31, 2017

Great! This should really help with compatibility.

This was referenced Nov 16, 2017
Closed
Merged
@spevans spevans force-pushed the pr_darwin_compatibility_tests branch from 20035f9 to 187a351 Compare November 18, 2017 07:51
@spevans
Copy link
Contributor Author

spevans commented Nov 18, 2017

@swift-ci please test

2 similar comments
@spevans
Copy link
Contributor Author

spevans commented Nov 18, 2017

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Nov 19, 2017

@swift-ci please test

@spevans spevans force-pushed the pr_darwin_compatibility_tests branch 2 times, most recently from a304027 to 53ffe2c Compare November 20, 2017 13:17
@spevans
DarwinComptibilityTests: Add Xcode Project
c4aad6e 
- Add shims and minor test fixes.

- Disable XDG tests as the xdgTestHelper does not currently work.

- Add handling for some tests requiring macOS 10.13 that run on 10.12
  when run against the sclf version of Foundation.
  This requires some of the tests to be duplicated and wrapped inside
  of a DARWIN_COMPATIBILITY_TEST conditional compile.
@spevans spevans force-pushed the pr_darwin_compatibility_tests branch from 53ffe2c to c4aad6e Compare November 20, 2017 13:18
@spevans
Copy link
Contributor Author

spevans commented Nov 20, 2017

@swift-ci please test

@spevans spevans changed the title [WIP] DarwinComptibilityTests: Add XCode Project DarwinComptibilityTests: Add Xcode Project Nov 20, 2017
@spevans
Copy link
Contributor Author

spevans commented Nov 20, 2017

I think this is ready for comments/merging now. If someone could test it locally on 10.13 that would be useful as I currently only run 10.12

@spevans spevans requested review from parkera, itaiferber and phausler and removed request for parkera November 20, 2017 15:36
@phausler
Copy link
Contributor

What else is remaining for this to be integrated into CI?

@ianpartridge
Copy link
Contributor

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Nov 21, 2017

Fixing the tests will be an ongoing effort and shouldn't prevent this being used for CI in some form. Simply compiling the tests against the native Foundation finds issues where the methods and therefore the tests have been implemented with the incorrect function signatures. I have fixed up all of the current issues in previous PRs so we should be able to proceed with a CI job that invokes an Xcode build of the tests but doesn't run them.

Im not actually sure how to use xcodebuild from the command line to just compile the tests, hopefully someone with more knowledge of Xcode can step in. There is also the issue that the Xcode will need the latest nightly toolchain installed although we might be able to get away with installing the last one that was built as the tests rarely use the latest swift features as soon as they are introduced.

I would suggest this being triggered as a manual job for now until we have confidence in how it works e.g., using please test darwin_compatibility or some such command.

As to the broken tests they mainly fall into the following groups:

  1. Differences between ObjC Foundation and SCLF that will always be different. These can just be disabled.

  2. Differences due to floating point representations and other cases where neither is wrong and we just need to decide on how compatible we want to be.

  3. Bugs in the SCLF implementation and therefore incorrect tests that need to be fixed.

  4. Bugs that are found in the native foundation. Again these will need to be disabled until they are fixed (if they are fixed) in a newer version of macOS.

@spevans
Copy link
Contributor Author

spevans commented Nov 22, 2017

@alblue Do you have any thoughts on this?

@jessesquires jessesquires mentioned this pull request Nov 23, 2017
Closed
@alblue
Copy link
Contributor

alblue commented Nov 23, 2017

I see a couple of crashes when running it against a newer version, including test_validateMutation_slice_mutableBacking_withUnsafeMutableBytes_lengthLessThanLowerBound and test_validateMutation_slice_customMutableBacking_withUnsafeMutableBytes_lengthLessThanLowerBound along with a slightly different one in test_URLComponents_JSON.

But I think we should migrate this in and then figure out how to fix these things incrementally to get to a clean bill of health.

The only comment I have is that the DarwinShims.swift at the top level could be misleading. Could we put that into the TestFoundation or DarwinCompatiblityTests directories instead?

Also, in the sections where you have defined the macro DARWIN_COMPATIBILITY_TESTS to stub out explicit frees that are handled by the autorelease pool (e.g. TestNSKeyedArchiver.swift) it might be better to use the DEPLOYMENT_RUNTIME_OBJC macro instead, because that's really what needs to gate the availability.

@spevans
DarwinCompatibilityTest fixes
7d963e1 
- Move DarwinShims.swift into DarwinCompatibilityTests subdir.

- Disable more tests that crash on Darwin native.

- Use DEPLOYMENT_RUNTIME_OBJC instead of DARWIN_COMPATIBILITY_TESTS
  to disable a deallocate() that isnt needed for AutoReleasing objects.
@spevans
Copy link
Contributor Author

spevans commented Nov 23, 2017

@alblue I fixed the issues you identified. TestNSKeyedArchiver.swift was also the only place I stubbed out the explicit free.

@spevans
Copy link
Contributor Author

spevans commented Nov 23, 2017

@swift-ci please test

@spevans
Copy link
Contributor Author

spevans commented Nov 23, 2017

@swift-ci please test

@alblue
Copy link
Contributor

alblue commented Nov 28, 2017

@swift-ci please test and merge

@swift-ci swift-ci merged commit 44eece9 into swiftlang:master Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itaiferber itaiferber Awaiting requested review from itaiferber

@parkera parkera Awaiting requested review from parkera

@phausler phausler Awaiting requested review from phausler

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

Successfully merging this pull request may close these issues.
6 participants
@spevans @parkera @phausler @ianpartridge @alblue @swift-ci