Generate boilerplate code for XCTest on linux using AST #164

aciidgh · 2016-03-02T16:34:23Z

About the implementation:

This creates a new target in debug.yaml called tests-ast which will dump ast files of all the test modules in .build/debug/TestsAST folder when swift test is run
There is a new module called ASTParser, which is fed the directory containing ast dumps and it gives back an array of TestModule which is :

public struct TestModule {
    struct Class {
        let name: String
        let testMethods: [String]
    }
    let name: String
    let classes: [Class]
}

This is then fed to generate() method which is mostly @czechboy0's implementation
Looks like the -dump-ast mode has some issues with #if directive and outputs imbalanced tree so I made changes in couple of tests.

Todo :

Refactor the changed made in describe()
Create and run ASTs only for linux
Should probably have a -disable-ast-gen option in case this behaves badly for some users
General refactoring
Tests

czechboy0 · 2016-03-02T16:59:18Z

Sources/ASTParser/generate().swift

+    }
+
+    try fputs("])\n\n", file)
+}


Newline please :)

Yeah, I need to go over and refactor a bit. just wanted to make it work and get opinion before ending up wasting a lot of time heading in wrong direction 😂

👍 Fair, I jumped right into the code, didn't read the description first 😇

gribozavr · 2016-03-02T17:06:12Z

Please don't use -dump-ast. It is an unstable format, an uncodumented flag, it can be even removed in release builds for code size reasons.

czechboy0 · 2016-03-02T17:08:17Z

Sources/Build/describe().swift

            try write("    other-args: ", args + otherArgs)
-            try write("    sources: ", module.sources.paths)
+            try write("    sources: ", module.sources.paths + (testGenFile != nil ? ["\(testGenFile!).swift"] : []))


Are you assuming that tests will never be compiled in Release mode? Otherwise you'd need to add the new generated files down in the .Release branch as well.

this was totally a work around just to see if it was working, as mentioned in description this file needs refactoring.... I don't even like what I did here 😆

aciidgh · 2016-03-02T17:13:55Z

Yeah, I understand. #156 (comment)
Also https://twitter.com/mxcl/status/704449586979409920

I'll update the PR when the stable form for swiftpm comes out. If its going to be long then should probably close this PR for now, @mxcl suggestions?

kylef · 2016-03-02T17:22:52Z

Package.swift

            name: "swift-build",
            dependencies: ["Get", "Transmute", "Build", "Multitool"]),
        Target(
            name: "swift-test",
-            dependencies: ["Multitool"]),
+            dependencies: ["Multitool", "ASTParser"]),            


Bunch of trailing line-space here. at the end.

fixed, thanks

briancroom · 2016-03-07T13:27:14Z

I mentioned this before but I want to bring it up again for further discussion.

I feel pretty strongly that doing the AST parsing and code generation here would couple swift test unnecessarily tightly to implementation details of XCTest. Even excluding future efforts to support other testing frameworks, just the fact that changes were required in this new code when swiftlang/swift-corelibs-xctest#40 was merged seems like a sign of trouble.

I realize that there is a lot of state involved in this process and that extracting the codegen could get tricky, but I'm envisioning that there could be a tool (ideally provided by swift-corelibs-xctest, I think) which could be used similar to this:

# Produce a Swift file with the test manifest for a module
xctest-gen module-manifest -module-name module1 -build-dir build/dir -o output/testmanifest1.swift input/testfile1.swift input/testfile2.swift

# Produce a Swift file with the test entrypoint
xctest-gen main -o output/testmain.swift output/testmanifest1.swift

SwfitPM would thus be responsible for choosing output filenames so that it could include those new files in the swiftc invocations etc., but the parsing and code generation responsibilities would be moved elsewhere.

Does this seem at all reasonable to you guys? Am I overestimating the value of splitting this out?

aciidgh · 2016-03-07T16:27:18Z

@briancroom I like the idea of this being in XCTest rather than in Swiftpm. I think it would a good idea if this is moved there once the new ast mode comes out

modocache · 2016-03-07T16:59:54Z

Totally agree with this being in XCTest.

mxcl · 2016-03-08T22:36:58Z

Does XCTest have a binary tool on Linux already?

mxcl · 2016-03-08T22:42:41Z

I'll update the PR when the stable form for swiftpm comes out. If its going to be long then should probably close this PR for now, @mxcl suggestions?

Someone has to write that code.

To me it is a minor detail if this is in XCTest or here. Really introducing a new binary for XCTest seems wrong. Especially since once we have Reflection we won't need it. To me it seems like SwiftPM should do this.

modocache · 2016-03-08T23:19:33Z

First of all, I'm really excited to see this test manifest tool come into existence, regardless of where it resides. Thanks for everyone's work on this! 💯

Here's my two cents:

A new binary for XCTest doesn't seem like a prohibitively expensive engineering investment. Perhaps I'm not understanding the complexity involved here? I am imagining we could add a command to the XCTest build script to generate an executable, which does not strike me as particularly difficult.
As we update what it means to be "a test" in XCTest, we'll need to change the tool that generates the list of tests. It's easier to do that if the tool and the test framework reside in the same repository. And the definition of what a test is does change: a recent example is allowing test methods to throws.
Once we do have reflection, I assume XCTest will be responsible for telling SwiftPM about which tests it defines. After all, the testing support evolution proposal points to a Swift protocol being used to determine the test manifest, not an AST. So it stands to reason that XCTest would implement that Swift protocol. At that point, it will be XCTest's responsibility to tell SwiftPM what tests are included in a test package. If we're headed to a future in which XCTest tells SwiftPM what the tests are, it seems a little odd for SwiftPM to determine the tests itself here--even if it is meant as a "pre-reflection" solution.

On the other hand, in the short-term, I suppose it would be easier to implement test manifest generation if it were baked into SwiftPM--we wouldn't have to worry about how the new XCTest tool and SwiftPM interface with one another. So I guess I could understand if we chose to include this tool in SwiftPM, under the following assumptions:

Reflection is coming very soon, and once it does this discussion is moot, or...
...we're optimizing for what's easier to implement right now.

And that's all I'll say on the matter! 😅 I'm sure whatever we decide will be great.

modocache · 2016-03-08T23:28:41Z

A new binary for XCTest doesn't seem like a prohibitively expensive engineering investment.

As I think more on this, I think the interaction between SwiftPM (which knows how to find the test files) and this tool (which knows about how to parse those files) may actually be quite complex. I suppose we could define a custom llbuild Tool for the new test manifest generator... but in any case, I think I am beginning to see how this is more complex than I first thought.

Sorry if my comment above gave the impression that I was trivializing the problem! 🙇

…e-removal-description Add roots to verbose description of `StaleFileRemovalCommand`

aciidgh added 9 commits March 2, 2016 12:55

AST generation for test modules

f25ad8a

Add module name and import path for ast gen

36b9d46

Move ASTs to a single dir

9682c09

Add parse AST method

82855d4

local commit

50ee3d1

First attempt at ast parser

c214c62

working on Linux

341b76c

Fix quote issue

b0a8e8a

Comment disabled testcases causing inbalanced AST

34c5fa9

czechboy0 reviewed Mar 2, 2016
View reviewed changes

kylef reviewed Mar 2, 2016
View reviewed changes

aciidgh force-pushed the tests-ast branch 5 times, most recently from 3126e4e to 8d57935 Compare March 2, 2016 17:32

Quick refactors

0d4eca3

aciidgh force-pushed the tests-ast branch from 8d57935 to 0d4eca3 Compare March 2, 2016 17:33

czechboy0 mentioned this pull request Mar 3, 2016

Automatic Linux test manifest generation #159

Closed

9 tasks

aciidgh closed this May 6, 2016

aciidgh deleted the tests-ast branch June 30, 2016 18:51

aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019

Merge pull request swiftlang#164 from neonichu/add-roots-to-stale-fil…

0281021

…e-removal-description Add roots to verbose description of `StaleFileRemovalCommand`

This was referenced Mar 10, 2016

[SR-32] Find way to make allTests required / audited on OS X swiftlang/swift-corelibs-xctest#411

Closed

[SR-710] Generate XCTestCaseProvider entries on Linux #5320

Closed

Generate boilerplate code for XCTest on linux using AST #164

Generate boilerplate code for XCTest on linux using AST #164

Uh oh!

Conversation

aciidgh commented Mar 2, 2016

Uh oh!

czechboy0 Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

czechboy0 Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Mar 2, 2016

Uh oh!

czechboy0 Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Mar 2, 2016

Uh oh!

kylef Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

aciidgh Mar 2, 2016

Choose a reason for hiding this comment

Uh oh!

briancroom commented Mar 7, 2016

Uh oh!

aciidgh commented Mar 7, 2016

Uh oh!

modocache commented Mar 7, 2016

Uh oh!

mxcl commented Mar 8, 2016

Uh oh!

mxcl commented Mar 8, 2016

Uh oh!

modocache commented Mar 8, 2016

Uh oh!

modocache commented Mar 8, 2016

Uh oh!

Uh oh!