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 tester implementation #146

Merged
merged 6 commits into from
Oct 26, 2018
Merged

improve tester implementation #146

merged 6 commits into from
Oct 26, 2018

Conversation

frairon
Copy link
Contributor

@frairon frairon commented Oct 1, 2018

Reimplementation of the tester to fix all issues (see issue #145)

@frairon frairon changed the title \#145 improve tester implementation #145 improve tester implementation Oct 1, 2018
@frairon frairon changed the title #145 improve tester implementation improve tester implementation Oct 1, 2018
@frairon frairon requested review from db7, SamiHiltunen and juandes and removed request for db7 October 1, 2018 13:28
@frairon frairon force-pushed the tester-refactoring branch 4 times, most recently from b52bed6 to a5c5a35 Compare October 5, 2018 09:22
}

// Scenario (2)
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Silly comment. You have this unused comment line :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

man.. how was this ever supposed to work :D


// MessageTracker tracks message offsets for each topic for convenient
// 'expect message x to be in topic y' in unit tests
type MessageTracker struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest bind the MessageTracker to a topic. That will simplify the interface.


// NextMessage returns the next message from the topic since the last time this
// function was called (or MoveToEnd)
func (mt *MessageTracker) NextMessage(topic string) (string, interface{}, bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: use Next() instead

}

// NextMessageRaw returns the next message in passed topic
func (mt *MessageTracker) NextMessageRaw(topic string) (string, []byte, bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: use NextRaw() instead

}

// ExpectEmpty ensures the topic does not contain more messages
func (mt *MessageTracker) ExpectEmpty(topic string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not necessary since once can call Next() until it returns false

Copy link
Contributor Author

@frairon frairon Oct 26, 2018

Choose a reason for hiding this comment

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

actually I'd leave the function, as it does more than just checking we're at the end. Also having a shortcut for the alternative loop that we would have to replae in all projects would be helpful.
but I'd rename it to ExpectAtEnd to have it consistend with MoveToEnd.
What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about replacing ExpectAtEnd() with something like mt.Offset() == mt.HWM() ?

and MoveToEnd() replacing with mt.Seek(mt.HWM())?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

// ExpectEmit ensures a message exists in passed topic and key. The message may be
// inspected/unmarshalled by a passed expecter function.
// DEPRECATED: This function is only to get some compatibility and should be removed in future
func (mt *MessageTracker) ExpectEmit(topic string, key string, expecter func(value []byte)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd remove this method.

tester/tester.go Outdated

// CreateMessageTracker creates a message tracker that starts tracking
// the messages from the end of the current queues
func (km *Tester) NewMessageTrackerFromEnd() *MessageTracker {
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: NewMessageTracker(topic string)

@db7 db7 merged commit 0ae3bc9 into master Oct 26, 2018
@db7 db7 deleted the tester-refactoring branch October 26, 2018 12:12
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