Skip to content

Automatic Linux test manifest generation #159

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.

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

Closed
wants to merge 1 commit into from
Closed

Automatic Linux test manifest generation #159

wants to merge 1 commit into from

Conversation

czechboy0
Copy link
Member

https://bugs.swift.org/browse/SR-710
(This is a reopened #156, see that for previous discussions)

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.

  • Please note that at the moment I'm using naive string-based parsing of the test classes to find test cases. Even though it's simple, I haven't yet found a case which it can't handle. Still, AST-based discovery is the long term goal here (and I hear @aciidb0mb3r is already working on it).
  • Note that the generation only happens on Linux, so if you want to test it out on a Mac just comment out the couple of ifdefs in describe() that have anything to do with test manifests.
  • By comparing running tests before and after the changes in this PR, I noticed that on Linux SwiftPM already wasn't running 2 tests that did run on OS X (124 Linux vs 126 OS X). This is just another reason to move to automatic generation, because missing tests might lead to a false sense of security. Now all 126 tests are ran on both platforms.

DONE

  • in build phase automatically generates XCTestMain.swift file (replacement for LinuxMain.swift) for each test product and a XCTestManifest.swift file for each module. The files are now generated in folder .build/xctestmanifests, meaning we don't add any user-visible files.
  • all test classes in XCTestMain are now scoped by their module, so there's no issue when two modules have the same test class name.
  • swiftpm's own tests are getting generated correctly, so this PR removes the manual Linux XCTestCaseProvider extensions and LinuxMain.
  • XCTestManifest.swift for each module are now correctly added to the module's sources. I had to add a function on Module to allow for adding sources after creation with an array of absolute paths.
  • originally this PR generated the test manifest files next to user's test cases, but that was later changed to generate these files in .build, to avoid users having to manage those files with SCM etc.
  • no migration planned, as @mxcl pointed out, we're allowed to make a breaking change like this this early in the development process

TODO with Questions

  • where should this code go? (A new module. @mxcl does testGen sound good to you?)
  • look into using an actual AST parser (e.g. swiftc -dump-ast), @aciidb0mb3r working on that
  • unit tests

@czechboy0
Copy link
Member Author

@mxcl Before I can go any further, I need some answers on #156 (comment) and #156 (comment).

@czechboy0
Copy link
Member Author

Hey @mxcl, I could use some guidance regarding this one and #164 before I update it with the latest XCTest syntax. Questions above, thanks!

@briancroom
Copy link
Contributor

@czechboy0 Sorry I never followed up on #156 (comment)

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.

I agree that pushing for uniformity is very much in-line with SwiftPM's goals, but I'm not thrilled about the idea of committing to an approach that would be entirely incompatible with packages interested in writing tests with tools like Quick or Spectre. It seems to me that the convention of providing an XCTestMain.swift function already goes a long way towards ensuring that packages work in an expected manner, without needing to be so strict as to make SwiftPM code generation a requirement for using swift test.

@czechboy0
Copy link
Member Author

Yeah I agree, but I was working with the assumption that the goals are to 1. get XCTest working on Linux without extra work required by the user, then 2. define a testing protocol to support non-XCTest runners. I feel like slowing down 1. will in turn also slow down 2.

But I don't think we need to prematurely try to optimize for a future work flow which hasn't yet been defined by a proposal. I understand it might lead to a bit of work being thrown out when step 2. is defined, but I'd prefer to have a great user experience at all stages of SwiftPM's development, not just after custom test runners are supported. Does that make sense?

@briancroom
Copy link
Contributor

Yup, it does make sense.

In any case, I'm mostly just glad to see this work coming together so quickly after the initial landing of the swift test code. There is plenty of time to extend the functionality for step 2.

@czechboy0
Copy link
Member Author

I'd like to resurrect this PR (somehow combined with @aciidb0mb3r's #164). It's still very annoying to have to make changes to tests twice plus I constantly see projects not testing everything on Linux simply because they forget to add the tests to the Linux manifests.

@mxcl Could you please chime in? I have a couple of questions in the comments above.

We also need to figure out where this code will live. I'd be for putting it into SwiftPM, while keeping in mind the testing protocol being designed at the moment - so that we can easily disable it for 3rd party testing frameworks once we can detect them in the future. That way users can just run tests on both platform and it just works.

@mxcl
Copy link
Contributor

mxcl commented Mar 22, 2016

Can we reiterate the questions and issues in this thread? The conversation has become a little all-over-the-place and I think it would help to clarify.

@gribozavr
Copy link
Contributor

  • Parsing Swift with an ad-hoc parsers is doomed to be perpetually buggy and become a maintenance liability.
  • Parsing -dump-ast is going to be fragile, or not even work at all, after we remove -dump-ast from production compilers to save code size.

=> The only viable solution (to me) seems to move the relevant test scanning code from SourceKit into libIDE, and provide a documented and supported driver option in the swift compiler.

@modocache
Copy link
Contributor

Can we reiterate the questions and issues in this thread? The conversation has become a little all-over-the-place and I think it would help to clarify.

I added a comment on SR-710 with a summary of my understanding here.

@czechboy0
Copy link
Member Author

@mxcl Sure, I was mainly wondering where you'd like to see this code.
@gribozavr I absolutely agree, this PR was mostly focused on building the infrastructure & generation, I was hoping the parser would be replaced by one that uses a stable and robust format. I guess we're blocked till your suggested changes are made.

I guess there's not much worth doing until we have access to the stable AST suggested by @gribozavr, so this PR is blocked by that work (does anyone have a SR-XXXX link to that work?)

@ddunbar
Copy link
Contributor

ddunbar commented Apr 2, 2016

I think we should start the discussion about getting the proper implementation of that kind of work off of the ground.

There are several important features for which we would like to have deeper integration with the Swift compiler:

  1. The tests-on-linux problem. I hoped we would be able to do this with reflection capabilities instead of test case source code inspection, but that seems too far off on the horizon to be something we can wait for.
  2. The design for Package.swift intended that we would mechanically validate the "leading package specification" to ensure it was machine editable. This is important for long term maintenance and for our ability to do things like auto-edit projects to add dependencies.
  3. We would like the package manager to assist with (target) dependency management, by being able to recognize missing dependencies and help with declaring them. We may want access to the AST for that, or other ways of interacting more deeply with the compiler (e.g., a richer output interface).

I'm not necessarily convinced the right answer for SwiftPM is to live directly atop the SourceKit functionality (since as others have mentioned, we should support 3rd-party frameworks), but we should start figuring out the right production quality interfaces to design and build here.

This is going to end up being a pretty big project, we should probably move the primary discussion to swift-build-dev. Someone will need to drive it, and I suspect neither @mxcl or myself will have time in the short term.

I think independently we may want to see if there is any other short-term mitigation for the tests-on-linux problem we can put in.

@modocache
Copy link
Contributor

@ddunbar Check out the discussion in SR-710 if you haven't already. @drewcrawford is of the opinion that Swift reflection is already at a point that we can use it to implement this feature. I haven't been following the reflection work closely so I can't say either way.

I'd love to help get this done, and I think the first step is to centralize the discussion. I personally would prefer SR-710, but if you'd like to start a mailing list thread, we can direct the discussion there.

@czechboy0
Copy link
Member Author

Closing this, as it's definitely not the way we'll take this feature. Although, if in the meantime you have a large test code base without a manifest, and would prefer to have it generated automatically, try this: https://github.com/czechboy0/SwiftXCTestLinuxNameGenerator

It's the naive, text-based parser & manifest generator, saved me hours of time already. Not robust, but definitely better than having to write the manifests manually 😊

@czechboy0 czechboy0 closed this May 6, 2016
aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019
…-tests

[BuildSystemTaskTests] Make sure first build system is closed before building with the second one
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.

6 participants