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

Integation tests for PgSQL connector #67

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

willyborankin
Copy link
Contributor

No description provided.

@ivanyu ivanyu self-assigned this Jan 8, 2021
ivanyu
ivanyu previously requested changes Jan 8, 2021
Copy link
Contributor

@ivanyu ivanyu left a comment

Choose a reason for hiding this comment

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

Haven't gone through all the code yet, but have some comments at this point already.

Comment on lines +203 to +206
if (project.hasProperty("tablePollIntervalMs")) {
//pass table.poll.interval.ms
systemProperty("integration-test.table.poll.interval.ms", project.hasProperty("tablePollIntervalMs"))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's a good idea to put the burden of choosing this value (or even knowing the value must be chosen) on whoever is going to run the tests. For example, I frankly have no idea what I should insert here without some research. We just need one good enough value set as a constant in tests somewhere.


public class AbstractPgSqlAwareIT extends AbstractIT {

static final Logger LOGGER = LoggerFactory.getLogger(PostgreSQLContainer.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static final Logger LOGGER = LoggerFactory.getLogger(PostgreSQLContainer.class);
static final Logger LOGGER = LoggerFactory.getLogger(AbstractPgSqlAwareIT.class);

return AdminClient.create(adminClientConfig);
}

private static Path setupPluginDir() throws Exception {
Copy link
Contributor

@ivanyu ivanyu Feb 11, 2021

Choose a reason for hiding this comment

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

Technically could be run in some @BeforeAll instead of @BeforeEach

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