Testing: godeps →vendoring, {command,file}_test.go

coreos / mayday

A diagnostics tool for capturing system state.

Apache License 2.0

81 stars 27 forks source link

Testing: godeps →vendoring, {command,file}_test.go #27

Closed tschuy closed 8 years ago

tschuy commented 8 years ago

Changes:

remove Godeps/, add vendor/
add tests:
- mayday_test.go: contains the test setup/teardown. Given mayday.go exists and this doesn't test that, is this perhaps not the right place to put this? What should the setup file be called?
- file_test, command_test: Are these tests comprehensive enough?
modify ./build: remove GOPATH export. Is this alright? It works on Travis ¯_(ツ)_/¯
bump Travis to Go 1.6, and remove 1.4/1.5. This is necessary with vendoring.
report coverage as part of testing, because why not. 0% → 78% for github.com/coreos/mayday/mayday!

tschuy commented 8 years ago

getJournals (in /mayday.go) has no tests, and that 78% is only for the mayday package, not main fwiw. It might be nice to make main have no code other than the command/file defintions, and move getJournals into the sorta-plugin architecture I described in #13.

tschuy commented 8 years ago

This might be a silly level of making things modular, but we could even have main load the list of files to copy, commands to run, and plugins to run from a config file depending on how abstract we want to get.

brianredbeard commented 8 years ago

Overall, LGTM. Curious from @colhom about the line comments above.

colhom commented 8 years ago

@tschuy it makes the diff more readable if the actual dependency shuffling happens entirely in it's own commit, so you don't have to ever look at the vendored dependency diff when reviewing

tschuy commented 8 years ago

@colhom ah yeah, that would've been good. @brianredbeard I've updated file_test.go, it should work on pretty much anything now.

colhom commented 8 years ago

@tschuy yeah you're right, all the changes were godep --> vendor or directly related, good enough for me.

I'd really like to see mayday.File keep pointers to files as io.ReadClosers. When state is expressed as a path string referring to an arbitrary file on disk, you're not really doing unit tests anymore.

tschuy commented 8 years ago

Okay, I'll look into getting mayday.File moved over to ReadClosers. Is this something I should do for this PR, or save it for another?

colhom commented 8 years ago

@tschuy up to you and how gnarly the refactor is (first pass says not very)

tschuy commented 8 years ago

Ok, this should be good now, I've refactored a little to use ReadClosers. @colhom @brianredbeard

tschuy commented 8 years ago

see https://github.com/coreos/mayday/pull/29