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

Improve bytescount middleware tests #80

Merged
merged 3 commits into from Jan 3, 2018

Conversation

Projects
None yet
3 participants
@donce
Copy link
Contributor

commented Jan 2, 2018

These changes started as a feature: we need to send session stats at the beginning of the session, not only after 1 minute.
It turns out that it's already the case - OpenVPN does this itself, it initiates first message at the beginning of session.

Anyway, these tests seem to be worth keeping - main purpose of middleware is to parse command and send stats, which was not tested before.

"github.com/mysterium/node/server/dto"
)

type SessionStatsSender interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

This struct look a bit excess, could be just custom-typed callback:

type SessionStatsSender func(sessionId string, bytesSent, bytesReceived int) error

This comment has been minimized.

Copy link
@donce

donce Jan 3, 2018

Author Contributor

In that case, you would also need mysteriumClient server.Client parameter - having 4 parameters in function does not seem readable to me.

This comment has been minimized.

Copy link
@donce

donce Jan 3, 2018

Author Contributor

Also, you would have to re-couple sessionId with middleware for this.

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 3, 2018

Member

Your factory can factory custom-typed function:

func NewSessionStatsSender(mysteriumClient server.Client, sessionId SessionId) SessionStatsSender {
     sessionIdString := string(sessionId)
     return SessionStatsSender(func() {
         mysteriumClient.SendSessionStats(sessionIdString, dto.SessionStats{})
     })
}
sessionId session.SessionId
sessionStatsSender SessionStatsSender
interval time.Duration
sessionId session.SessionId

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 2, 2018

Member

Maybe sessionId type could be wrapped into SessionStatsSender.
With such approach we could achieve total decoupling:

  • bytescount parsing
  • parsed data sending

This comment has been minimized.

Copy link
@donce

donce Jan 3, 2018

Author Contributor

Great suggestion! 💰💰💰
Done, now middleware doesn't have to know application specifics!

@tadovas

This comment has been minimized.

Copy link
Member

commented Jan 3, 2018

@donce Please update your pull request with master changes

@donce donce force-pushed the refactor/MYST-197-session-creation branch from c3ca8e0 to 61a7336 Jan 3, 2018

BytesSent: bytesSent,
BytesReceived: bytesReceived,
})
}

func NewSessionStatsSender(mysteriumClient server.Client) SessionStatsSender {
return &clientSessionStatsSender{mysteriumClient}
func NewSessionStatsSender(mysteriumClient server.Client, sessionId string) SessionStatsSender {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 3, 2018

Member

Pass SessionId object here

This comment has been minimized.

Copy link
@donce

donce Jan 3, 2018

Author Contributor

Done

"github.com/mysterium/node/server/dto"
)

type SessionStatsSender interface {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 3, 2018

Member

Your factory can factory custom-typed function:

func NewSessionStatsSender(mysteriumClient server.Client, sessionId SessionId) SessionStatsSender {
     sessionIdString := string(sessionId)
     return SessionStatsSender(func() {
         mysteriumClient.SendSessionStats(sessionIdString, dto.SessionStats{})
     })
}
@Waldz

Waldz approved these changes Jan 3, 2018

@Waldz Waldz merged commit d94e2d8 into master Jan 3, 2018

@Waldz Waldz deleted the refactor/MYST-197-session-creation branch Jan 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.