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

Additional Heartbeat Reload Tests #8228

Merged
merged 4 commits into from
Oct 3, 2018
Merged

Conversation

andrewvc
Copy link
Contributor

@andrewvc andrewvc commented Sep 5, 2018

This PR attempts to improve the testing and testability of heartbeat in the following ways:

  1. Adds a mocked test of the Monitor type
  2. Minimizes the required mockable area by introducing a PipelineConnector type that is easier to mock than a Pipeline.
  3. Introduces a number of useful mocks for future tests potentially.

@andrewvc andrewvc added in progress Pull request is currently in progress. Heartbeat labels Sep 5, 2018
return fmt.Sprintf("Monitor not loaded, plugin is disabled")
}

type MonitorPluginInfo struct {

Choose a reason for hiding this comment

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

exported type MonitorPluginInfo should have comment or be unexported

"github.com/elastic/beats/libbeat/common"
)

type PluginDisabledError struct{}

Choose a reason for hiding this comment

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

exported type PluginDisabledError should have comment or be unexported

}
}

func (m *Monitor) Stop() {

Choose a reason for hiding this comment

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

exported method Monitor.Stop should have comment or be unexported

return nil
}

func (m *Monitor) Start() {

Choose a reason for hiding this comment

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

exported method Monitor.Start should have comment or be unexported

@andrewvc andrewvc force-pushed the hbreload-test branch 3 times, most recently from 42df612 to f766544 Compare September 19, 2018 18:08
@andrewvc andrewvc changed the title [WIP] Additional Heartbeat Reload Tests Additional Heartbeat Reload Tests Sep 20, 2018
@andrewvc andrewvc added review and removed in progress Pull request is currently in progress. labels Sep 20, 2018
@andrewvc
Copy link
Contributor Author

This is now ready for review

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

Code looks good do me but one of the new tests seems to fail. I wonder if it's the timeout or something else.

require.Equal(t, 1, len(pipelineConnector.clients))
pcClient := pipelineConnector.clients[0]

time.Sleep(time.Duration(100 * time.Millisecond))
Copy link
Member

Choose a reason for hiding this comment

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

Sleeps in tests ;-)

@andrewvc
Copy link
Contributor Author

@ruflin fixed and improved in 35a9170

The issue was that the mock impl was doing a copy into a zero size slice with the right capacity, instead of an empty slice of the right capacity.

I also improved the timeout code to exit early by spinning on the condition. Wouldn't mind some review on this, my understanding is that the go scheduler is not safe to spin in this situation and still make forward-progress without explicit sleeps in it, even on a single core system. Is that correct? I added microsecond sleeps in the loop to make sure we yield.

Looking at golang/go#10958 seems to confirm that aspect of the go scheduler behavior.

@ruflin
Copy link
Member

ruflin commented Sep 28, 2018

It seems on Jenkins Mac build this causes a problem in libbeat: https://beats-ci.elastic.co/job/elastic+beats+pull-request+multijob-darwin/483/beat=libbeat,label=macosx/console I can't really see how this is related to the interface change in libbeat TBH. Perhaps it's flaky but it's not one I have seen before.

@andrewvc andrewvc merged commit 0417f98 into elastic:master Oct 3, 2018
@andrewvc andrewvc deleted the hbreload-test branch October 3, 2018 15:55
andrewvc added a commit to andrewvc/beats that referenced this pull request Oct 3, 2018
* Adds a mocked test of the Monitor type
* Minimizes the required mockable area by introducing a PipelineConnector type that is easier to mock than a Pipeline.
* Introduces a number of useful mocks for future tests potentially.

(cherry picked from commit 0417f98)
@andrewvc andrewvc added the v6.5.0 label Oct 3, 2018
andrewvc added a commit that referenced this pull request Oct 4, 2018
* Adds a mocked test of the Monitor type
* Minimizes the required mockable area by introducing a PipelineConnector type that is easier to mock than a Pipeline.
* Introduces a number of useful mocks for future tests potentially.

(cherry picked from commit 0417f98)
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.

3 participants