Added -help option to sourcekitd-test #8746

felix91gr · 2017-04-13T20:07:59Z

What the PR includes

Addresses one of the current starter bugs, adding the feature mentioned in the title.

I've added a FIXME: in the printHelp function to suggest a possible expansion for its functionality (allowing to hide/include certain options from the Help Text). I don't think that's necessary for this command, but could be useful if we want it to have as many options as Driver.cpp has.

Where has this been tested

I've compiled this in Linux, using a slightly modified version of the default presets. I used #8485 and #8189, and added a cmake option to compile sourcekitd-test. It passed the buildbot_linux, smoketest tests on my pc 👍

Resolves SR-1421.

felix91gr · 2017-04-14T00:47:27Z

@swift-ci please smoke test

slavapestov · 2017-04-17T05:31:58Z

@swift-ci Please test

nkcsgexi · 2017-04-17T18:25:54Z

Can we add a test to ensure this dump the -h option itself?

felix91gr · 2017-04-17T20:00:43Z

🤔 sure... I can add that

felix91gr · 2017-04-18T03:27:17Z

Do we have a documentation on the testing tools? I'm talking about the kind of tests found in swift/test

EDIT

Found it! FileCheck and Testing Guide

felix91gr · 2017-04-18T21:38:29Z

@nkcsgexi I made this test and it passed. Should I test something else as well?

// Basic check that usage works
//
// RUN: %sourcekitd-test -help | %FileCheck %s
//
// CHECK: USAGE: sourcekitd-test [options] <inputs>

nkcsgexi · 2017-04-18T21:42:18Z

Thank you! @swift-ci please smoke test

felix91gr · 2017-04-18T22:25:57Z

Thank you too🙂 Btw... 🤔 I made a new test, for checking that entering an unknown argument prints an error message and suggests using -help. But running the 9xxx tests takes a long time 😆. Is there a way to run just *one* test? Or to run just the SourceKit ones?

…

On Tue, Apr 18, 2017 at 6:43 PM Xi Ge ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#8746 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALNBJ5KmSGq0cNxpoH7psIDS40BgkWTHks5rxS6AgaJpZM4M9JRu> .

nkcsgexi · 2017-04-18T22:30:11Z

@felix91gr could you push your test and ask swift CI to test it for you?

felix91gr · 2017-04-18T22:35:08Z

Sure! I'll push it 🙂 I can't trigger swift CI though... I'm a new-ish member of the community, so I haven't been given access to it. (I already tried in my first comment, that was when I learned)

…

On Tue, Apr 18, 2017 at 7:30 PM Xi Ge ***@***.***> wrote: @felix91gr <https://github.com/felix91gr> could you push your test and ask swift CI to test it for you? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALNBJ_ww0b1PB65nVTsiOjPHDbKHVv5Mks5rxTmagaJpZM4M9JRu> .

nkcsgexi · 2017-04-18T22:41:08Z

@swift-ci please smoke test

felix91gr · 2017-04-18T22:47:12Z

please smoke test

Thanks! ^^

nkcsgexi · 2017-04-18T22:52:11Z

@felix91gr whenever you need to trigger CI, just comment on the pull request to ask some one to trigger it for you. I'm sure many are glad to help 🙂

felix91gr · 2017-04-19T00:00:47Z

It failed, this is the output:

Command Output (stderr):
--
error: unknown argument: -this_option_does_not_exist
Use -h or -help for assistance
FileCheck error: '-' is empty.
FileCheck command line:  /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/buildbot_incremental/llvm-macosx-x86_64/./bin/FileCheck /Users/buildnode/jenkins/workspace/swift-PR-osx-smoke-test/branch-master/swift/test/SourceKit/Misc/wrong_arguments.swift

--

From that, only the first two lines are the output of sourcekitd-test. That's the correct output (what we are testing, in fact). I have problems interpreting the FileCheck error though... what does...

FileCheck error: '-' is empty. mean?

This is the original test, btw:

// RUN: %sourcekitd-test -this_option_does_not_exist | %FileCheck %s
//
// CHECK: error: unknown argument:
// CHECK: Use -h or -help for assistance

Should I use the complete line? (like // CHECK: error: unknown argument: -this_option_does_not_exist)

I thought CHECK didn't enforce the line to be exactly equal... I wanted to give freedom to have any erroneous option in the test, just in case.

(Meta)

🤔 it failed only on OS X... Now that I look at it, the entire SourceKit test suite is disabled in Linux.

Ah, right. We haven't yet merged Norio Nomura's PR #8485. Well, I'll test it on my machine as well then (I'm using Linux) 👍

felix91gr · 2017-04-19T00:45:37Z

Yup, the CHECK string must be matched exactly. That must've been the error.

@nkcsgexi I've submitted a correction. Can you call Swift-CI please? :)

nkcsgexi · 2017-04-19T00:46:12Z

@swift-ci please smoke test

felix91gr · 2017-04-19T02:22:36Z

🤔 both failed... that's weird.

Well, I'll restart my setup and keep at it.

felix91gr · 2017-04-20T03:01:09Z

@benlangmuir or @nkcsgexi, I think I fixed the tests this time... sorry to bother you, could you run a smoke test please? :)

nkcsgexi · 2017-04-20T06:12:59Z

@swift-ci please smoke test

felix91gr · 2017-04-20T22:50:52Z

@benlangmuir or @nkcsgexi... Sorry to bother you again. This time I've tested the changes using a local version of FileCheck. It should work. If it doesn't... 🤔 we'll see.

(please call CI ^^)

nkcsgexi · 2017-04-20T22:54:08Z

@swift-ci please smoke test

felix91gr · 2017-04-21T00:07:47Z

Wohoo! The wrong_arguments.swift test passed! I'm making it work :D

Next up, I'll fix whatever is wrong with the usage.swift test ✊

felix91gr · 2017-04-21T00:45:07Z

I think I know what's the problem. I'll upload the change.

felix91gr · 2017-04-21T00:59:09Z

The problem was that I thought the -help command returned 0, because it doesn't trigger an error. I was wrong.

In the design of sourcekitd-test, any option that makes the program not test a SourceKit request, returns 1. If the "happy path" would be to parse a request and perform a SourceKit test on it, then anything but the happy path returns 1. Therefore, both of my tests (wrong_arguments and usage should expect a 1 for result, and therefore should start with {// RUN: not sourcekitd-test ...}. Which now, they do :) (I tried it first with wrong_arguments, and it passes)

So, for (hopefully) last time... @nkcsgexi could you call CI? :)

nkcsgexi · 2017-04-21T01:01:01Z

@swift-ci please smoke test

felix91gr · 2017-04-21T01:39:53Z

Aw yeah! It passed! :D

felix91gr · 2017-04-21T02:33:55Z

It passed all the tests now!
Thanks for your patience, @nkcsgexi 😄

It was a dumb mistake, as it always is :D

felix91gr · 2017-04-21T17:45:25Z

Should we merge this? Should I add something else? 🙂

nkcsgexi · 2017-04-21T17:53:19Z

Could you squash all commits into one?

felix91gr · 2017-04-21T18:03:42Z

Oh, good idea. Will do.

…

On Fri, Apr 21, 2017 at 2:54 PM Xi Ge ***@***.***> wrote: Could you squash all commits into one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ALNBJ70iBsgGIJ7KgxKLOnUr4BDMCeFWks5ryO08gaJpZM4M9JRu> .

felix91gr · 2017-04-21T18:32:43Z

Done! :)

nkcsgexi · 2017-04-21T18:59:55Z

👍 @swift-ci please smoke test and merge

felix91gr · 2017-04-21T21:05:30Z

🤔 seems like nothing happened...

nkcsgexi · 2017-04-21T21:11:53Z

Let's try again. @swift-ci please smoke test

felix91gr · 2017-04-21T23:34:42Z

Ahhh... you know, probably when asked for a test + merge process, it puts the task way back into the queue because the tasks before it where called using the previous state of the repo :D (w/o the merge)

If that's the case, then it should take a while sometimes, depending on how busy CI is 🤔

akyrtzi · 2017-04-22T00:58:14Z

utils/build-presets.ini

@@ -708,7 +708,7 @@ install-llbuild
 install-swiftpm
 install-xctest
 install-prefix=/usr
-swift-install-components=autolink-driver;compiler;clang-builtin-headers;stdlib;swift-remote-mirror;sdk-overlay;license
+swift-install-components=autolink-driver;compiler;clang-builtin-headers;stdlib;swift-remote-mirror;sdk-overlay;license;sourcekit-inproc


This seems irrelevant, is this change intentional ?

Oh. I didn't notice. It's a leftover of one of my cherrypicks from Nomura's PR (#8485). Helped me test things in linux.
It can, and probably should, be removed... even though that PR has already been aprooved. You can do it if you want, or just wait for me (atm I'm not at the computer) :)

Remove the change when you can, and afterwards we can merge your improvements.

Oki doke 🙂👍🏻

felix91gr · 2017-04-22T02:38:37Z

@akyrtzi done 😉

Added "-help" option to Options.td Added also: * Added "OPT_HELP" case in the main TestOptions.cpp switch. It uses the llvm options help functionality to provide up-to-date help * Additional "Use -help for assistance" at the end of the error message that appears when calling an unknown option Added their following tests, respectively:: * usage.swift, and * wrong_arguments.swift Extra: FIXME: in TestOptions::printHelp, suggesting a possible expansion for the printHelp option (details in file)

felix91gr · 2017-04-22T18:15:31Z

Updated

I deleted everything that was not needed:
- A //WIP comment that I made in TestOptions.cpp, and
- The ;sourcekit-inproc install option in the build presets that I used for testing.
I also added:
- A brief documentation on the new tests, and
- A newline that was missing at the end of TestOptions.cpp

I think we're ready for a test and merge. @nkcsgexi or @akyrtzi could you please call CI again? :)

akyrtzi · 2017-04-24T21:33:11Z

@swift-ci smoke test and merge

akyrtzi · 2017-04-24T22:02:32Z

@swift-ci smoke test

felix91gr · 2017-04-24T23:01:31Z

What a weird thing... why did it fail the smoke test & merge, but passed the smoke test alone?

The only thing I can think of is that I haven't set gpg keys for my Github account. That could be it 🤔

felix91gr · 2017-04-24T23:02:53Z

No, wait... it says "wrong SHA". 🤔
What made the SHA change?

felix91gr · 2017-04-24T23:17:24Z

😄

Thanks for the help and the patience, guys! @nkcsgexi and @akyrtzi specially :)

nkcsgexi · 2017-04-24T23:36:08Z

@felix91gr congrats for your first Swift contribution! 👍

jrose-apple requested review from nkcsgexi and benlangmuir April 17, 2017 18:18

nkcsgexi approved these changes Apr 18, 2017

View reviewed changes

felix91gr force-pushed the master branch from 0f534fa to 4d7b5c4 Compare April 21, 2017 18:31

akyrtzi reviewed Apr 22, 2017

View reviewed changes

felix91gr force-pushed the master branch 3 times, most recently from 3e301c2 to 51521ef Compare April 22, 2017 02:37

felix91gr force-pushed the master branch from 51521ef to 98844ca Compare April 22, 2017 18:05

felix91gr force-pushed the master branch from 98844ca to 042fccb Compare April 22, 2017 18:07

akyrtzi merged commit be12528 into swiftlang:master Apr 24, 2017

swift-ci mentioned this pull request Apr 13, 2017

[SR-1421] 'sourcekitd-test -help' should print help, list available request types #44030

Closed

Added -help option to sourcekitd-test #8746

Added -help option to sourcekitd-test #8746

Uh oh!

Conversation

felix91gr commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What the PR includes

Where has this been tested

Uh oh!

felix91gr commented Apr 14, 2017

Uh oh!

slavapestov commented Apr 17, 2017

Uh oh!

nkcsgexi commented Apr 17, 2017

Uh oh!

felix91gr commented Apr 17, 2017

Uh oh!

felix91gr commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

EDIT

Uh oh!

felix91gr commented Apr 18, 2017

Uh oh!

nkcsgexi commented Apr 18, 2017

Uh oh!

felix91gr commented Apr 18, 2017 via email

Uh oh!

nkcsgexi commented Apr 18, 2017

Uh oh!

felix91gr commented Apr 18, 2017 via email

Uh oh!

nkcsgexi commented Apr 18, 2017

Uh oh!

felix91gr commented Apr 18, 2017

Uh oh!

nkcsgexi commented Apr 18, 2017

Uh oh!

felix91gr commented Apr 19, 2017

(Meta)

Uh oh!

felix91gr commented Apr 19, 2017

Uh oh!

nkcsgexi commented Apr 19, 2017

Uh oh!

felix91gr commented Apr 19, 2017

Uh oh!

felix91gr commented Apr 20, 2017

Uh oh!

nkcsgexi commented Apr 20, 2017

Uh oh!

felix91gr commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nkcsgexi commented Apr 20, 2017

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

nkcsgexi commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

nkcsgexi commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017 via email

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

nkcsgexi commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 21, 2017

Uh oh!

felix91gr commented Apr 13, 2017 •

edited

Loading

felix91gr commented Apr 18, 2017 •

edited

Loading

felix91gr commented Apr 20, 2017 •

edited

Loading

felix91gr commented Apr 21, 2017 •

edited

Loading

felix91gr commented Apr 21, 2017 •

edited

Loading