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

Feature/route finder single route #568

Conversation

ivcosla
Copy link

@ivcosla ivcosla commented Sep 10, 2019

Closes #567

Copy link

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

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

Good work. However, how can we test this?

MaxHops: maxHops,
// FindRoutes returns routes from source skywire visor to destiny, that has at least the given minHops and as much
// the given maxHops.
func (c *apiClient) FindRoutes(ctx context.Context, rts [][2]cipher.PubKey, opts *RouteOptions) ([][]routing.Route, error) {

Choose a reason for hiding this comment

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

Have the following changes:

  • Create a new type type Path []routing.Hop
  • Use the following as the output: map[[2]cipher.PubKey][]routing.Path

@ivcosla
Copy link
Author

ivcosla commented Sep 11, 2019

Good work. However, how can we test this?

We can run integration tests, no matter which one as long as we test that routes are being created. They are working on my tests. Basically run the following:

  1. checkout both repositories (skywire and skywire-services) to feature/route-finder-single-route
  2. on skywire-services run make integration-build; make integration-run-generic; make integration-startup
  3. run curl --data {'"recipient":"'$PK_A'", "message":"Hello Joe!"}' -X POST $CHAT_C, if such message arrives to visor_a it's succesful

For unit testing the API however I think it would be better to write them on the server side.

@evanlinjin
Copy link

@nkryuchkov please give this a review and see if we can merge this into your work so far. Thanks!

Copy link
Collaborator

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Great job!

return nil, err
}
res := bytes.NewBuffer(b1)
res.WriteString(":") // nolint
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although both WriteString and Write of bytes.Buffer always return nil as an error, I would explicitly check for an error here and below.

@nkryuchkov
Copy link
Collaborator

@evanlinjin I've already merged it into my work

@evanlinjin
Copy link

@evanlinjin I've already merged it into my work

Should we close this PR then?

@nkryuchkov
Copy link
Collaborator

@evanlinjin If it's to be merged in my work only, then yes.

@evanlinjin
Copy link

Already merged into #562

@evanlinjin evanlinjin closed this Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants