Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Represent :Capability type as *capnp.Client. #232

Merged
merged 8 commits into from
May 9, 2022
Merged

Conversation

lthibault
Copy link
Collaborator

Addresses #192.

@lthibault lthibault requested a review from zenhack April 28, 2022 23:22
@lthibault lthibault self-assigned this Apr 28, 2022
@lthibault lthibault force-pushed the enhancement/untyped-caps branch from b18e3d9 to a185ba0 Compare April 28, 2022 23:28
@lthibault lthibault marked this pull request as ready for review April 29, 2022 01:18
@lthibault
Copy link
Collaborator Author

No idea why tests are failing. They pass on my machine 😕

Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things in line, but otherwise LGTM; thanks for doing this. Obviously we also need to figure out why the tests are failing.

capnpc-go/capnpc-go.go Outdated Show resolved Hide resolved
capnpc-go/capnpc-go.go Outdated Show resolved Hide resolved
capnpc-go/capnpc-go.go Outdated Show resolved Hide resolved
internal/aircraftlib/aircraft.capnp.go Show resolved Hide resolved
pogs/pogs_test.go Outdated Show resolved Hide resolved
rpc/receiveranswer_test.go Outdated Show resolved Hide resolved
rpc/receiveranswer_test.go Outdated Show resolved Hide resolved
@lthibault
Copy link
Collaborator Author

Some minor things in line, but otherwise LGTM; thanks for doing this.

My pleasure, this was a fun one!

Obviously we also need to figure out why the tests are failing.

I'll take a look at that now. I'm admittedly quite stumped, though, as I can't reproduce the failure on my machine.

@lthibault lthibault requested a review from zenhack April 29, 2022 14:08
@lthibault lthibault changed the title WIP. Represent :Capability type as capnp.Interface. Represent :Capability type as *capnp.Client. Apr 29, 2022
@lthibault
Copy link
Collaborator Author

AFAICT the tests are actually passing, but the CI script is (sometimes?) interpreting calls to t.Logf as errors.

Notice the last line 👇

=== RUN   TestMessage_ResetReadLimit
Error:     readlimit_test.go:70:    m := &Message{TraverseLimit: 42}
Error:     readlimit_test.go:72:    m.canRead(42) -> true
Error:     readlimit_test.go:74:    m.ResetReadLimit(8)
Error:     readlimit_test.go:76:    m.canRead(8) -> true
    readlimit_test.go:81: 
Error:     readlimit_test.go:84:    m := &Message{TraverseLimit: 42}
Error:     readlimit_test.go:86:    m.canRead(40) -> true
Error:     readlimit_test.go:88:    m.ResetReadLimit(8)
Error:     readlimit_test.go:92:    m.canRead(9) -> false
    readlimit_test.go:95: 
Error:     readlimit_test.go:98:    m := &Message{TraverseLimit: 42}
Error:     readlimit_test.go:100:    m.canRead(42) -> true
Error:     readlimit_test.go:102:    m.ResetReadLimit(8)
Error:     readlimit_test.go:104:    m.canRead(8) -> true
    readlimit_test.go:109: 
Error:     readlimit_test.go:112:    m := &Message{TraverseLimit: 42}
Error:     readlimit_test.go:114:    m.canRead(40) -> true
Error:     readlimit_test.go:116:    m.ResetReadLimit(8)
Error:     readlimit_test.go:120:    m.canRead(9) -> false
    readlimit_test.go:123: 
Error:     readlimit_test.go:126:    m := new(Message)
Error:     readlimit_test.go:128:    m.ResetReadLimit(0)
Error:     readlimit_test.go:132:    m.canRead(0) -> true
    readlimit_test.go:135: 
Error:     readlimit_test.go:138:    m := new(Message)
Error:     readlimit_test.go:140:    m.ResetReadLimit(0)
Error:     readlimit_test.go:144:    m.canRead(1) -> false
--- PASS: TestMessage_ResetReadLimit (0.00s)

@zenhack
Copy link
Contributor

zenhack commented Apr 29, 2022

Hm, looking at this I don't think the t.Log is the issue: see https://github.com/capnproto/go-capnproto2/runs/6230315382?check_suite_focus=true#step:4:1616; it looks like the race detector is what's complaining.

Otherwise LGTM, but we need to figure out what's going on there.

@lthibault
Copy link
Collaborator Author

https://github.com/capnproto/go-capnproto2/runs/6230315382?check_suite_focus=true#step:4:1616

Oh weird, I must have scrolled right over that. Why isn't that happening on my end, I wonder ... 🤔

Digging into this now.

@zenhack
Copy link
Contributor

zenhack commented May 9, 2022

It's pretty clear this isn't the source of the data races, so I'm going to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants