[WIP] Automatic Linux test manifest generation #156
Conversation
|
Wow, this is amazing! Thanks for this, and thanks for linking to it on SR-710. 👍 |
| //imports | ||
| try fputs("import XCTest\n", file) | ||
| try metadata.dependencies.forEach { | ||
| try fputs("@testable import \($0)\n", file) |
There was a problem hiding this comment.
Sorry if I'm missing something, but do we need to import dependencies at all here? As far as I understand this...
- ...we're writing a test manifest that will be part of the test package that declares all the test cases and methods, correct? So we can reference the test cases directly, without an import.
- ...the test cases and their test methods won't reference their dependencies. So we don't need to import them.
Am I missing something?
There was a problem hiding this comment.
Ah, I think you're right. I just got too influenced by LinuxMain.swift's structure. I removed it, thanks!
|
Again, this is really great. I'm very impressed with how much boilerplate this eliminates, even in SwiftPM's codebase alone! 😍 One thing I'd like to request is a way of "opting-out" of this feature. Some third-party testing frameworks have their own custom systems for building a I understand that SwiftPM is only concerned with XCTest integration for now, but I think allowing for an opt-out now would make it easier to support more frameworks in the future. Thoughts? 🙌 |
|
@modocache I thought about this, because I support the proposition of making SwiftPM open to alternative testing frameworks in the future. The way the generator works now is that
So as far as I understand, if you don't have any XCTest subclasses in your test, nothing will get generated. Is that enough to not break Quick-based test modules? Otherwise we can talk about ways of opting out. |
|
Now the newly generated files are correctly added to each module's sources, so please give it a try on your projects. Verify that the same number of tests are run on OS X and Linux. This works for swiftpm itself. No need for manual XCTestCaseProvider extensions anymore. @modocache What do you think about my question re where we should keep these files? I'm leaning towards generating them in |
| /// Splits string with any character in passed-in string of delimiters | ||
| public func splitWithCharactersInString(delimiters: String) -> [String] { | ||
| let stops = Set(delimiters.characters) | ||
| return self.characters.split { stops.contains($0) }.map(String.init) |
There was a problem hiding this comment.
No need to wrap the stop.contains in a closure, could pass the contains func directly.
- characters.split { stops.contains($0) }.map(String.init)
+ characters.split(stops.contains).map(String.init)There was a problem hiding this comment.
Unfortunately that's trying to match the wrong split and doesn't compile. I'd have to do
characters.split(isSeparator: stops.contains).map(String.init)
which reads worse to me.
There was a problem hiding this comment.
But thanks for pointing it out, I always forget you can just pass the function in. 👍 I only tried this after you commented here, couldn't get it to work though.
Ingenious!! Sounds great. And I admit I've yet to think about projects that include a mix of XCTestCase and Quick tests--I'm not even sure our mechanism works for those right now, so in we definitely shouldn't worry about it here. Thanks!! Very excited for this pull request. |
|
@modocache Cheers, thanks for your comments and advice here 👍 |
|
I defer to @mxcl on where to keep the files. My vote is tentatively for |
|
@czechboy0 @modocache Awesome work! I wanted to mention that I've been working on AST generation and parsing for the same bug. So far I've been able to generate AST of tests modules by adding a "tests-ast" target in debug.yaml to generate ASTs via swift-build-tool and then parse it when swift-test is run. Its working but needs more work in parsing I guess and then file generation. Anyway @czechboy0 do you think we can combine to efforts here? |
|
@modocache Agreed, especially since the manifests are clearly anti-DRY, the truth is in fact in the tests themselves, so I view these as just intermediate build products. |
|
@czechboy0 great! Awesome. Also, I feel the generator and parser belongs in Multitool rather than Build, thoughts? |
|
Very possible, I built it from the integration point out, so that's why it's in Build now, but I'm happy to move it wherever you and @mxcl think is best. |
|
I made the change so that now instead of generating test manifests and LinuxMain in the Tests directory among the user's test files, we now generate them in |
|
@modocache If this gets merged, how are we going to handle the documentation in Swift XCTest? Technically, when using SwiftPM, users will be discouraged from adding |
…o speed up PR feedback
|
Nice work. We definitely need to use the AST. For example: this was I have been discussing this with some others at Apple and we feel that the best way to approach this is to add a new AST output mode to It should be an undocumented option, that SwiftPM uses exclusively on Linux. Thus the AST output can be the minimum we require. The reason we cannot depend on the output from
IMO, doing nothing is fine. We're an early product and have the liberty to make breaking changes.
A new module is good IMO. I have no name in my head yet. |
|
Also, we'll need to watch this PR #158 |
I looked at that and the generator will require only a small change in the files it produces. It already has all the information it needs (test class names, test function names and test module name) in the generator, so those PRs can be merged and then I'll make the change here.
How does
Do you have an idea of who will work on that? I understand that will be a prerequisite of @aciidb0mb3r 's AST parser working properly, thus of this PR (unless you'd willing to merge the current version with the string-based parser, which, as you mentioned, doesn't respect #ifdef's). |
|
@czechboy0 this is really cool work! I'm very glad you've opened this PR with such a convincing demonstration of how viable this approach is right now. Here are a few thoughts I have:
|
|
@briancroom Thanks!
That sounds very reasonable, however I'm wondering what the general direction of these files is. The advantage of what you're suggesting is that existing packages won't have to change, but I also consider that a long term burden, because we'll be creating divergence already. I think pushing everyone to the same approach, whatever that is, is preferable in the long term. But if not breaking existing packages becomes a requirement here, your suggestion is the best one I've seen so far.
Absolutely, I thought about this too before, I haven't come up with a good name yet.
Although I'd prefer that too, currently the generator is using and modifying the |
|
@czechboy0 Hey can you squash your commits ? I'm going to start working on integrating this with ast parser |
…ain way of adding them to the product is too late, they need to be in each of their module already before that
…dule, neither it adds it to LinuxMain
…eir module. I don't see any harm in always scoping, in fact it is clearer which class comes from which module. And since this is generated people won't be looking at it much anyway. removed manual linux XCTestCaseProvider extensions [Parsing] handle test classes with modifiers [Parsing] now checking for trailing () to verify test function (well, -ish). Also ignoring LinuxTestManifest.swift when going through test files, but that's just for good practice
[Test manifest] removed an old manual Linux test manifest
[Generator] We don't actually need to import the dependencies in the LinuxTestManifest files.
[Design] Moved Array extension back into the only file that uses it.
[Generator] Don't generate LinuxMain.swift if no test classes are to be written.
[Parser] Handle e.g. "func testMe(){" where there's no whitespace before {
[Describe] Now properly adding manifests to their module source files, thus everything should be working on first build/test. Turns out not that much hackery was necessary.
[Fix] Immutability warning fixed
[Typo] fix
[Experiment] Generating files in .build/.xctestmanifests instead of next user's files
Make xctestmanifests not hidden in .build
No need to check for already added manifest file to a module because these files are not mixed with user files anymore
Using the new protocol Buildable's isTest
Change if to guard Add \n at EOF Update SourceLayouts.md
# Conflicts: # Tests/LinuxMain.swift # Tests/PackageDescription/PackageTests.swift
|
@aciidb0mb3r My squashing attempts went all over the place, so I reopened the PR on a new branch. Please follow #159 from now on. Apologies everyone. |
…d-result Revert "Change DB API to find constant key IDs."
https://bugs.swift.org/browse/SR-710
This is first stab at automatically generating Linux test manifests. Generation seems to be working correctly, but I haven't written any tests yet, so it's not ready to merge. I wanted to present this early to get feedback.
describe()that have anything to do with test manifests.DONE
XCTestMain.swiftfile (replacement forLinuxMain.swift) for each test product and aXCTestManifest.swiftfile for each module. The files are now generated in folder.build/xctestmanifests, meaning we don't add any user-visible files.Moduleto allow for adding sources after creation with an array of absolute paths..build, to avoid users having to manage those files with SCM etc.testGenfor now. Name suggestions welcome)TODO with Questions
swiftc -dump-ast), @aciidb0mb3r working on that