Pushes test coverage just over 50% #18

nathany · 2016-10-13T01:54:55Z

resolves merge conflicts in #15 from @drewwells

Pushes test coverage just over 50%

I broke all C calls into a separate file "wrap.go" and wrapped some types in Go types. This way, fsevents.go does not import "C". In addition, testing can not import C, so it makes it easier to test the different methods. There's several extra functions added to allow better inspection of FSEventStreamRef for testing the correct setup of File Event stream.

FSEventStreamRef should probably become a struct that wraps the underlying C.FSEventStreamRef. This would remove some of the ugly GetStreamRefXXX. I'm also open to making most of the GetStreamRefXXX calls private, since it's unlikely consumers will actually call these. We should likely rely on the GetStreamRefPaths for reporting the actual paths being watched.

Also, nothing in this actually tests a live filesystem event.

It does test:

a real device ID is being targeted

registry does what it claims to do

FSEvents UUID can be pulled for the device ID

FSEventStream is correctly setup and watching specified paths

nathany · 2016-10-13T03:02:08Z

Travis CI is having some problems. https://www.traviscistatus.com/

Btw, I've been force pushing travis ci configs to this branch. Yell at me to stop if you need me to.

nathany

I'm too 💤 and too inexperienced with FSEvents to do a proper review, but I left a few comments. Of course I'm happy to make such minor revisions on this branch -- or even leave them for after.

Would be great if @samjacobson and/or someone else could take a look.

nathany · 2016-10-13T03:04:25Z

example/main.go

@@ -13,18 +14,24 @@ import (
 )

 func main() {
-	dev, _ := fsevents.DeviceForPath("/tmp")
+	path := "/tmp"


Maybe this should be os.TempDir()?

nathany · 2016-10-13T03:11:13Z

example/main.go

 	es.Start()
 	ec := es.Events

+	fmt.Println("Device UUID", fsevents.GetDeviceUUID(dev))


could just use log.Println since everything else in the file does

nathany · 2016-10-13T03:11:37Z

example/main.go

+	path := "/tmp"
+	dev, err := fsevents.DeviceForPath(path)
+	if err != nil {
+		log.Println("Failed to retrieve device for path:", err)


should this be log.Fatal?

nathany · 2016-10-13T03:15:20Z

fsevents.go

-
-	es.Events <- events
-}
-
 // LatestEventID returns the most recently generated event ID, system-wide.
 func LatestEventID() uint64 {


not everything in wrap.go is unexported, so maybe just move this function there

nathany · 2016-10-13T03:27:18Z

fsevents.go

+// You can provide your own event channel if you wish (or one will be
+// created on Start).
+//
+// es := &EventStream{Paths: []string{"/tmp"}, Flags: 0}


This code snippet doesn't render so well in godoc. not sure if it could be made into a (working) example? though an example need not be executed, so long as it compiles

http://127.0.0.1:8080/pkg/github.com/fsnotify/fsevents/

nathany · 2016-10-13T03:30:32Z

wrap.go

+
+	es.stream = setupStream(paths, es.Flags, callbackInfo, since, es.Latency, es.Device)
+
+	started := make(chan struct{})


this was the addition that caused the merge conflict, see #18

nathany · 2016-10-13T03:31:41Z

wrap.go

+
+// CFArrayLen retrieves the length of CFArray type
+// See https://developer.apple.com/library/mac/documentation/CoreFoundation/Reference/CFArrayRef/#//apple_ref/c/func/CFArrayGetCount
+func CFArrayLen(ref C.CFArrayRef) int {


maybe we don't want to export this?

nathany · 2016-10-13T03:32:12Z

wrap_test.go

+	paths := []string{"/a", "/b"}
+	ref := setupStream(paths, 0, 0, eid, time.Duration(0), did)
+
+	t.Log(GetStreamRefDescription(ref))


not sure if we want to keep this log in the tests?

nathany · 2016-10-13T03:38:38Z

Once Travis CI is up and working again, we can force a rebuild and see if I got the config right for macOS. I'm planning to setup https://codecov.io/ as well.

codecov-io · 2016-10-13T15:13:48Z

Current coverage is 43.03% (diff: 41.30%)

No coverage report found for master at cb6277d.

Powered by Codecov. Last update cb6277d...ce7b5c6

nathany · 2016-10-13T17:10:10Z

Test coverage for this branch can be viewed here: https://codecov.io/gh/fsnotify/fsevents/branch/testmock

I've asked @stevepeak why the discrepancy between coverage % on codecov.io and running it on the command line.

samjacobson · 2016-10-13T18:54:49Z

I'm under the gun with another project at the moment. I can take a look
early to mid next week if that's at all helpful.

Cheers

Sam

On Fri, Oct 14, 2016 at 6:10 AM, Nathan Youngman notifications@github.com
wrote:

Test coverage can be viewed here: https://codecov.io/gh/
fsnotify/fsevents/branch/testmock

I've asked @stevepeak https://github.com/stevepeak why the discrepancy
between coverage % on codecov.io and running the command line.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#18 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABMJvcITecow-mw0UANA6E5Vgv5CGZL1ks5qzmXygaJpZM4KVZu0
.

nathany · 2016-10-13T18:58:14Z

Thanks Sam. That would be wonderful.

nathany · 2016-10-18T02:32:12Z

@drewwells Do you have the time to continue working on this?

drewwells · 2016-10-18T06:07:44Z

Yeah I can look at it tomorrow

drewwells · 2016-10-19T15:14:37Z

It doesn't appear that I have push access to this PR, I'll open a PR against it then

nathany · 2016-10-19T18:29:47Z

Sorry @drewwells. I've adjusted the settings.

fix vet errors in fsevtCallback separate all parts that call C code into separate wrap file test registry functions test creating of paths test inspects StreamRef to ensure setupStream created it correctly

nathany · 2016-10-25T17:37:52Z

Travis CI is very slow at starting the macOS tests. We are enabled to use CircleCI now, but the yml file needs configuring. https://circleci.com/gh/fsnotify/fsevents

This is what I had to do before, but it may have changed since.
fsnotify/fsnotify@c733143

https://circleci.com/docs/ios-builds-on-os-x/

drewwells · 2016-10-25T20:10:55Z

Why is codecov failing? This MR should be significantly increasing coverage from master.

I can look into the circleci issue, that should probably be in a separate PR so this one doesn't get too overwhelming.

nathany · 2016-10-25T20:53:46Z

I don't know what the deal is with codecov. Let's see what happens on the next pull request.

nathany · 2016-10-25T23:05:09Z

@samjacobson I went ahead and merged this, but feel free to review and open PRs or issues if you see something that can be improved. Thanks!

nathany force-pushed the testmock branch from c9b5177 to 38f6279 Compare October 13, 2016 02:03

nathany mentioned this pull request Oct 13, 2016

next steps #17

Open

5 tasks

nathany force-pushed the testmock branch from 38f6279 to 45bba88 Compare October 13, 2016 02:25

nathany force-pushed the testmock branch from 45bba88 to 4a4bdb3 Compare October 13, 2016 03:07

nathany commented Oct 13, 2016

View reviewed changes

nathany force-pushed the testmock branch from 4a4bdb3 to 67b5020 Compare October 13, 2016 13:01

drewwells and others added 4 commits October 25, 2016 11:21

Pushes test coverage just over 50%

30242f1

fix vet errors in fsevtCallback separate all parts that call C code into separate wrap file test registry functions test creating of paths test inspects StreamRef to ensure setupStream created it correctly

configure Travis CI

8854f53

travis: add codecov

1100866

code review fixes

ce7b5c6

nathany force-pushed the testmock branch from 99149c0 to ce7b5c6 Compare October 25, 2016 17:21

nathany merged commit b319f15 into master Oct 25, 2016

nathany deleted the testmock branch October 25, 2016 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pushes test coverage just over 50% #18

Pushes test coverage just over 50% #18

nathany commented Oct 13, 2016

nathany commented Oct 13, 2016 •

edited

Loading

nathany left a comment

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany Oct 13, 2016

nathany commented Oct 13, 2016

codecov-io commented Oct 13, 2016 •

edited

Loading

nathany commented Oct 13, 2016 •

edited

Loading

samjacobson commented Oct 13, 2016

nathany commented Oct 13, 2016

nathany commented Oct 18, 2016

drewwells commented Oct 18, 2016

drewwells commented Oct 19, 2016

nathany commented Oct 19, 2016

nathany commented Oct 25, 2016 •

edited

Loading

drewwells commented Oct 25, 2016

nathany commented Oct 25, 2016

nathany commented Oct 25, 2016


		es.stream = setupStream(paths, es.Flags, callbackInfo, since, es.Latency, es.Device)

		started := make(chan struct{})

Pushes test coverage just over 50% #18

Pushes test coverage just over 50% #18

Conversation

nathany commented Oct 13, 2016

nathany commented Oct 13, 2016 • edited Loading

nathany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathany commented Oct 13, 2016

codecov-io commented Oct 13, 2016 • edited Loading

Current coverage is 43.03% (diff: 41.30%)

nathany commented Oct 13, 2016 • edited Loading

samjacobson commented Oct 13, 2016

nathany commented Oct 13, 2016

nathany commented Oct 18, 2016

drewwells commented Oct 18, 2016

drewwells commented Oct 19, 2016

nathany commented Oct 19, 2016

nathany commented Oct 25, 2016 • edited Loading

drewwells commented Oct 25, 2016

nathany commented Oct 25, 2016

nathany commented Oct 25, 2016

nathany commented Oct 13, 2016 •

edited

Loading

codecov-io commented Oct 13, 2016 •

edited

Loading

nathany commented Oct 13, 2016 •

edited

Loading

nathany commented Oct 25, 2016 •

edited

Loading