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

Run integration tests with pytest #919

Merged
merged 15 commits into from
Mar 26, 2020

Conversation

danielmitterdorfer
Copy link
Member

With this commit we add a proof-of-concept implementation of how
integration tests could look like with py.test instead of shell scripts.
For the proof of concept we migrate the integration tests for the list
subcommands and source builds.

It also spins up an Elasticsearch metrics store and checks that any
preconditions (basically Docker being present and up and running) are
met. Finally, it also integrates into CI by writing JUnit XML files for
test results.

Relates #736

With this commit we add a proof-of-concept implementation of how
integration tests could look like with py.test instead of shell scripts.
For the proof of concept we migrate the integration tests for the list
subcommands and source builds.

It also spins up an Elasticsearch metrics store and checks that any
preconditions (basically Docker being present and up and running) are
met. Finally, it also integrates into CI by writing JUnit XML files for
test results.

Relates elastic#736
@danielmitterdorfer danielmitterdorfer added enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc. discuss Needs further clarification from the team labels Feb 26, 2020
@danielmitterdorfer danielmitterdorfer self-assigned this Feb 26, 2020
@danielmitterdorfer
Copy link
Member Author

Unfortunately we don't see any Rally output in the PR checks. Therefore I have raised #923.

@danielmitterdorfer danielmitterdorfer added this to the 1.5.0 milestone Mar 3, 2020
@danielmitterdorfer danielmitterdorfer marked this pull request as ready for review March 3, 2020 16:18
@danielmitterdorfer danielmitterdorfer removed the request for review from drawlerr March 4, 2020 08:45
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Great work and foundation!

I love the iterative approach (i.e. gradually replace the functionality in integration-test.sh) and the fact that this is so readable and easy to read makes me happy.

I left a comment/suggestion to use Template rather than replacing strings (and a FYI comment).

def copy_config(name):
f = ConfigFile(name)
io.ensure_dir(f.rally_home)
with open(f.target_path, "w", encoding="UTF-8") as target:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to just read src into contents with something:

with open(f.source_path, "r", encoding="UTF-8") as src:
    contents = src.read()

and then replace using Template's native capabilities, to avoid pure string based replacements, i.e.

from string import Template
with open(f.target_path, "w", encoding="UTF-8") as target:
    target.write(Template(contents).substitute(USER_HOME=f.user_name)

AFAIK ${identifier}-like template strings are supported out of the box.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Much better. Addressed in eb03450.

return
else:
c.close()
time.sleep(0.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR, but going forwards should we start embracing the https://github.com/jd/tenacity library, which seems to be becoming the standard in retrying in Python? It offers a lot flexibility in what to retry with boolean operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have some timeout here just so our it tests dont run forever on a PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am reluctant adding additional dependencies for such small pieces of functionality but I added a two minute timeout in 3537d0b.

Copy link
Contributor

@hub-cap hub-cap left a comment

Choose a reason for hiding this comment

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

This is great work, LGTM. nothing but super minor nits.

return
else:
c.close()
time.sleep(0.5)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have some timeout here just so our it tests dont run forever on a PR?

it/__init__.py Outdated
if process.run_subprocess_with_logging("docker") != 0:
raise AssertionError("Docker is required to run integration tests.")
if process.run_subprocess_with_logging("docker ps") != 0:
raise AssertionError("Docker daemon must be up and running to run integration tests.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need both of these checks? We can just say "docker must be installed and running" if docker ps fails

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I combined the checks in 7786bdc.


class ConfigFile:
def __init__(self, config_name):
self.user_home = os.path.expanduser("~")
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use pathlib's Path.home() instead of the ~ since tilde expansion is black magic

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd tend to be consistent across our code base and keep this idiom.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM

@danielmitterdorfer danielmitterdorfer merged commit b33da19 into elastic:master Mar 26, 2020
@danielmitterdorfer
Copy link
Member Author

Thanks for your reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves the status quo :misc Changes that don't affect users directly: linter fixes, test improvements, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants