golang / sublime-build

The official Sublime Text package for Go build system integration.
BSD 3-Clause "New" or "Revised" License
344 stars 46 forks source link

Recommended way to run Benchmarks #13

Closed grange74 closed 8 years ago

grange74 commented 8 years ago

I'm really enjoying using this new sublime package. The only very minor issue i've found so far is that it was not obvious how to run Benchmarks. After looking through all the wikis i can see that there are a lot of configuration options which can allow you to do it (a few examples below) but nothing in the documentation actual references Benchmarks directly. Anyway i thought having something specific for how to run Benchmarks might be helpful for others who are using this package. I'm happy to help add it to the documentation.

In golang.sublime-settings:

{
     "test:flags": ["-v", "-bench=."]
}

In Default.sublime-commands:

{
    "caption": "Go: Test (Benchmark)",
    "command": "golang_build",
    "args": {
        "task": "test",
        "flags": ["-bench=."]
    }
}
jbuberel commented 8 years ago

@grange74 I like that suggestion! How about if I were to add that as a new section under "Command Flags":

https://github.com/golang/sublime-build/blob/master/docs/configuration.md#command-flags

Or is there a better place to put this?

wbond commented 8 years ago

@jbuberel Do you think we should consider adding a new build variant by default for this?

The downside of overriding test:flags is that benchmarks always run with tests. The addition to the command palette would not override test, but it would not be presented as a build variant.

grange74 commented 8 years ago

A build variant would be ideal. After running the test:flags option a few times, i realised that it doesn't work too well as typically you either wanted the tests to run or the benchmarks, not both.

grange74 commented 8 years ago

Similar to adding a command to the palette, using key bindings works well and is my preferred option after a build variant.

Example:

{
    "keys": ["super+ctrl+b"],
    "command": "golang_build",
    "args": {
        "task": "test",
        "flags": ["-bench=."]
    }
}

Added using instructions in the Key Bindings Example wiki

jbuberel commented 8 years ago

@wbond I like that suggestion - adding a new build variant that is specific to Go Generate.

@grange74 If Will and I were to provide some guidance on how to go about adding the feature, would you be willing to make time to prepare a CL for it?

grange74 commented 8 years ago

Yes, i'd like to have a go at making the changes. I'm keen to contribute to the Go community. Just a warning I will be a bit slow as i'm new to Gerrit, haven't done much python and doing it in my spare time. Thankfully it seems you've provided good documentation on contributing and particularly getting an environment setup to run the tests, which is usually one of the main hurdles.

Should i implement both the Benchmark and Generate build variants? (obviously as separate changes)

grange74 commented 8 years ago

I have submitted my changes for review, i hope that i have followed all the guidelines correctly.

jbuberel commented 8 years ago

@grange74 Awesome - taking at look at it now. I've added myself and @wbond as code reviewers:

https://go-review.googlesource.com/#/c/16851/

jbuberel commented 8 years ago

Overall, this looks great.

One question I have is this: The command that would be executed is go test -bench=., which will be executed in the directory containing the file currently being edited.

But as per the go help testflag documentation, you can optionally specify a regular expression to match the set of benchmarks you'd like to have executed:

    -bench regexp
        Run benchmarks matching the regular expression.
        By default, no benchmarks run. To run all benchmarks,
        use '-bench .' or '-bench=.'.

How would a user configure this command to override (via project level configuration, perhaps) that default with a regular expression?

Second question: The go test command also offers two other benchmarking-related options:

    -bench regexp
        Run benchmarks matching the regular expression.
        By default, no benchmarks run. To run all benchmarks,
        use '-bench .' or '-bench=.'.

Do you think it would be useful to include documentation examples showing users how to also specify these flags/values when running their benchmarks? Perhaps adding a short section here:

https://github.com/golang/sublime-build/blob/master/docs/configuration.md#using-command-flags-to-specify-build-run-and-install-targets

jbuberel commented 8 years ago

@grange74 You probably already figured this out, but just in case:

When addressing feedback from the code review, edit/commit locally as you normally would. When your next patch set is ready, just run

git mail

And it will append the next patch set to your in-progress CL.

grange74 commented 8 years ago

Thank you both for the quick and detailed feedback.

@jbuberel i hope that i understood your questions correctly. 1st question: this is probably also related to Will's comments in gerrit. My changes don't currently cater for this. I will be changing it so that it only appends -bench=. if the -bench flag hasn't already been set. This should allow users to specify a regular expression to only run certain benchmarks. 2nd question: i'll add some extra documentation.

@wbond regarding the tests, i would like to write some but embarrassingly i can't seem to work out how to run them. It could be my setup (OSX, ST3, Python 2.7.10) or just lack of python experience. I've read the comments on the development wiki several times and googling but haven't had much luck. When i run it from the CLI i get:

Traceback (most recent call last):
File "tests.py", line 13, in <module>
import sublime
ImportError: No module named sublime
jbuberel commented 8 years ago

@grange74 aargh - I copy/pasted the wrong snippet into the second code block of my comment. It should have been:

    -benchmem
        Print memory allocation statistics for benchmarks.

    -benchtime t
        Run enough iterations of each benchmark to take t, specified
        as a time.Duration (for example, -benchtime 1h30s).
        The default is 1 second (1s).

I just want to better understand whether users will be able to customize the command to include either of these additional flags?

grange74 commented 8 years ago

@jbuberel No worries. I understood what you meant. Yes, definitely I've been testing with these 2 flags

{
    "benchmark:flags": ["-benchmem", "-v", "-benchtime", "10s"]
}
wbond commented 8 years ago

@grange74 I just wanted to ping you on my feedback that I left at https://go-review.googlesource.com/#/c/16851/

grange74 commented 8 years ago

@wbond I'd made the golang_build.py and usage.md changes locally as per your feedback and had started to write tests but i haven't managed to be able to run the tests so i've held off pushing my updates. I still can't get past the 'ImportError: No module named sublime' i mentioned in earlier comments. Can you please let me know how you run the tests?

wbond commented 8 years ago

Tests must be run from within Sublime Text since the package heavily interacts with the Sublime Text API. If you are getting an error about sublime not being defined, that makes me think you are trying to use your system (or another) Python install.

In short, you need to install Package Coverage and use the Run Tests command. The https://github.com/golang/sublime-build/blob/master/docs/development.md docs have some more info. We'd be more than happy to accept feedback or improvement on those if anything could be more clear. If you do have changes, they'll just need to go in a different CL.

grange74 commented 8 years ago

Thanks a lot for the clear response. It seems so obvious in hindsight. While i did install that package, I didn't know what to do with it and obviously I didn't scroll far enough down the Package Coverage documentation page, my apologies. Once i get more spare time i will see if i can suggest some improvements to development.md for complete SBT plugin newbies likes me.

One thing i did have to do to get all my tests to pass was to create the 'pkg' and 'bin' folders under the 'go_projects' otherwise i got errors like the following. I noticed that the 'go_projects2' has these folders.

 ======================================================================
  ERROR: test_build (Golang Build.dev.tests.GolangBuildTests)
  ----------------------------------------------------------------------
  Traceback (most recent call last):
    File "/Users/nic/Library/Application Support/Sublime Text 3/Packages/Golang Build/dev/tests.py", line 50, in setUp
      for entry in os.listdir(full_path):
  FileNotFoundError: [Errno 2] No such file or directory: '/Users/nic/Library/Application Support/Sublime Text 3/Packages/Golang Build/dev/go_projects/pkg'

I'm not sure if this is something i did wrong or whether i should create a separate issue for this?

wbond commented 8 years ago

@grange74 It looks like I missed adding the .git-keep files for go_projects. I'll submit a CL for that.

grange74 commented 8 years ago

@wbond It took me a while but all changes are finally ready for another review https://go-review.googlesource.com/#/c/16851/

wbond commented 8 years ago

I'm on holiday right now, but will follow up with that next week.

wbond commented 8 years ago

@jbuberel I reviewed Patch Set 2 and it LGTM. I can't approve, however.

grange74 commented 8 years ago

Great. Thanks to you both for your guidance and patience.