You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/08/17 14:53:40 UTC
[GitHub] [tinkerpop] good41898 opened a new pull request, #1785: Export Result interface
good41898 opened a new pull request, #1785:
URL: https://github.com/apache/tinkerpop/pull/1785
The `Result` struct does not allow for any assigning of data, which makes it impossible to mock out for testing. While the `ResultSet` interface and its methods can be mocked out, they must match the original function signature, and return the `Result` type. Since you cannot assign to the unexported `Result` type, it is not possible to mock out returning data from the various methods of `ResultSet`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218543187
@good41898 Hi there, thanks for using the Golang driver, cool to see people are putting it into things now.
First off, did you need some help with running the tests? If you need more assistance there feel free to jump into the TinkerPop Discord and send us some more details, you can @ myself or yang to get our attention quicker.
On the note of this code. I am unsure why this is required. The typical technique from my understanding of Golang (which I will admit is minimal) is to mock by making your own interface, for example: https://github.com/apache/tinkerpop/blob/master/gremlin-go/driver/gorillaTransporter_test.go#L35-L66
Please correct my if I am wrong in my understanding.
Anyway I will approve your workflow to run since @xiazcy has suggested it might create some errors.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] codecov-commenter commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218555177
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1785?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1785](https://codecov.io/gh/apache/tinkerpop/pull/1785?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c3645dc) into [master](https://codecov.io/gh/apache/tinkerpop/commit/b6442d2c2f628b26af940f7eac0d7c9a300f6673?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b6442d2) will **not change** coverage.
> The diff coverage is `86.66%`.
```diff
@@ Coverage Diff @@
## master #1785 +/- ##
=======================================
Coverage 64.06% 64.06%
=======================================
Files 23 23
Lines 3679 3679
=======================================
Hits 2357 2357
Misses 1154 1154
Partials 168 168
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1785?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [gremlin-go/driver/result.go](https://codecov.io/gh/apache/tinkerpop/pull/1785/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvcmVzdWx0Lmdv) | `89.65% <84.61%> (ø)` | |
| [gremlin-go/driver/resultSet.go](https://codecov.io/gh/apache/tinkerpop/pull/1785/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvcmVzdWx0U2V0Lmdv) | `80.18% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] lyndonbauto commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218603421
Looks like tests are passing. I am OK with moving this through if there is an actual need, just need you to get back to me on this part
> On the note of this code. I am unsure why this is required. The typical technique from my understanding of Golang (which I will admit is minimal) is to mock by making your own interface, for example: https://github.com/apache/tinkerpop/blob/master/gremlin-go/driver/gorillaTransporter_test.go#L35-L66
before we proceed @good41898
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] good41898 commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
good41898 commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218657790
You are correct that the typical technique for mocking is by making your own interface!
To give an example of the issue, I have abstracted the gremlin-go driver into an interface like this:
`GremlinGoDriver interface {
Submit(traversalString string) (gremlingo.ResultSet, error)
Close()
}`
Which contains the two methods of the driver that I use. While unit testing, I am able to mock out any methods (like the Submit() and Close() method) as long as the function signatures for the mocked methods match the signatures of the actual methods. Where I am running into issues is when trying to mock returned data from a `ResultSet` object. I am able to create a new struct containing the result set so I can mock those methods (like the ResultSet.All() method) as well, like this:
```
type MockResultSet struct {
ResultSet
}
func (m MockResultSet) All() ([]*Result, error) {
var res []*Result
return res
}
```
However, since the function signatures must match, this mocked All() method must return a Result object, which I cannot assign data to as the result field is non-exported.
Let me know if this is helpful/ makes sense. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] xiazcy commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218349343
Hi, @good41898, thank you for raising the issue. We will take a look.
Also a quick note before diving into the issue, please make sure all tests pass locally with your changes. As it currently stands, just exporting the field in the `Result` struct without changing any reference to it will generate errors. Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] good41898 closed pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
good41898 closed pull request #1785: Export Result interface
URL: https://github.com/apache/tinkerpop/pull/1785
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] good41898 commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
good41898 commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1219752173
closing pull request, will open a new one based off of `3.5-dev`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] good41898 commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
good41898 commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218491022
Thanks @xiazcy ! I was able to successfully run unit tests locally for Result and ResultSet packages, but am running into connection issues with the local gremlin server while running `./run.sh 3.6.1` even though I have confirmed both the gremlin server container and integration test container are up and running, and on the same docker network. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] good41898 commented on pull request #1785: Export Result interface
Posted by GitBox <gi...@apache.org>.
good41898 commented on PR #1785:
URL: https://github.com/apache/tinkerpop/pull/1785#issuecomment-1218136366
https://issues.apache.org/jira/browse/TINKERPOP-2785
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org