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

Move common RabbitMQ testing code to its own package #7106

Merged
merged 2 commits into from
May 17, 2018

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented May 15, 2018

New testing package provides a RabbitMQ mocked management API that can be used by all tests in the module.

Some additional assertions have been added to detect missing testing files.

DataDir: "../_meta/testdata/",
}

func Server(t *testing.T, c ServerConfig) *httptest.Server {

Choose a reason for hiding this comment

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

exported function Server should have comment or be unexported

DataDir string
}

var DefaultServerConfig = ServerConfig{

Choose a reason for hiding this comment

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

exported var DefaultServerConfig should have comment or be unexported

"testing"
)

type ServerConfig struct {

Choose a reason for hiding this comment

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

exported type ServerConfig should have comment or be unexported

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.

LGTM. Needs a make fmt.

@jsoriano jsoriano force-pushed the rabbitmq-testing-package branch 2 times, most recently from 5001195 to b9a2ea9 Compare May 16, 2018 17:14
"os"
)

func GetIntegrationConfig() map[string]interface{} {

Choose a reason for hiding this comment

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

exported function GetIntegrationConfig should have comment or be unexported

"os"
)

func GetIntegrationConfig() map[string]interface{} {

Choose a reason for hiding this comment

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

exported function GetIntegrationConfig should have comment or be unexported

@jsoriano
Copy link
Member Author

I have moved all common code for integration tests to the rabbitmq/testing package too.

@@ -17,7 +17,8 @@ def get_importable_lines(go_beat_path, import_line):
imported_lines.append(module_import)

module_path = join(path, module)
metricsets = [m for m in listdir(module_path) if isdir(join(module_path, m)) and m not in ["_meta", "vendor"]]
ignore = ["_meta", "vendor", "testing"]
metricsets = [m for m in listdir(module_path) if isdir(join(module_path, m)) and m not in ignore]
Copy link
Member Author

@jsoriano jsoriano May 16, 2018

Choose a reason for hiding this comment

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

@ruflin @exekias we are going to have to make a decision about where to place the code shared by metricsets of the same module. If we place it as a subdirectory of the module then we have to add an exception here so it is not imported, and then the module has to be named always the same in all modules.

If we want to avoid collisions with testing package maybe we could call this mstest (ms from metricset) or something like this as in principle two metricsets are only going to import this package from its own module.

If we decide to place these packages in other subdirectory we should also decide where to place it, an option could be under _meta, but I don't like it so much. In any case I'd place them inside the module.

Opinions?

(for context, related with discussion here #7059 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

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

(Btw, I love that tests have detected this issue in this PR 😄)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if under _meta would even work as I remember something golang ignores everything which starts with and underline.

What about mtest instead of mstest as it's the the things which are shared across the module? Reason I prefer mtest or mstest over testing is one the conflict and the other part is that I think the likelyhood of having a conflicting metricset name is lower.

Copy link
Member

Choose a reason for hiding this comment

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

I think the reason the tests catched this is because we had some issue around it in the past ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'm good with mtest too.

Copy link
Member

Choose a reason for hiding this comment

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

lets move forward with it and fix it when we break things :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed to mtest.

@jsoriano
Copy link
Member Author

jenkins, test this again, please

@ruflin ruflin merged commit a1def32 into elastic:master May 17, 2018
stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018
New testing package provides a RabbitMQ mocked management API that can be used by all tests in the module.

Some additional assertions have been added to detect missing testing files.
stevea78 pushed a commit to stevea78/beats that referenced this pull request May 20, 2018
New testing package provides a RabbitMQ mocked management API that can be used by all tests in the module.

Some additional assertions have been added to detect missing testing files.
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