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

Support E2E testing #3896

Merged
merged 2 commits into from
Jun 14, 2019
Merged

Support E2E testing #3896

merged 2 commits into from
Jun 14, 2019

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Jun 10, 2019

What does this PR do?

Adds the ability to test against the real Datadog Agent collector.

Implementation

A new command test is added to the ddev env namespace to facilitate the testing of only e2e tests e.g. any tests decorated with @pytest.mark.e2e. By default, it will assume an environment & Agent has already been spun up via ddev env start ... so you can rapidly iterate by repeated invocations to test. A flag -ne/--new-env will instruct the command to also run ddev env start ... and ddev env stop ... for you. Just like ddev test ..., it accepts an optional list of checks to test, each with optional envs specified. If no checks are selected, it will test what checks have changed in git compared to master (example) and force the behavior of --new-env. You can also use the ddev test ... command directly if you pass the new --e2e flag, though you can only test one environment per invocation.

A new global fixture dd_agent_check is introduced. This fixture yields a function which accepts a config and arbitrary keyword arguments. The function will call ddev env check ... using the configuration passed in as well as any additional args we specified, and feed the results to our standard aggregator stub, which it will then return. Here is a full example:

@pytest.mark.e2e
def test_e2e(dd_agent_check, some_instance):
    aggregator = dd_agent_check(some_instance, rate=True)

Note that it will not call the ddev within the virtualenv, but rather the ddev used to invoke the test run since the CLI requires many more dependencies than we require for testing.

Finally, as we may not want to run e2e tests for all environments (especially due to the increased CI pressure), it is necessary to mark them for e2e test automation in tox.ini. For example:

[tox]
...
envlist =
    py{27,37}-{1.0,5.2,latest}

[testenv]
description =
    py27: e2e ready
...

would mark the envs py27-1.0, py27-5.2, and py27-latest as supporting e2e tests. When the Agent supports Python 3, we will make the Python versions actually influence what the spun up Agents use.

Additional Notes

  • In order to see for example monotonic counts or rates in the Agent aggregator output you logically need to run the check more than once using the rate flag. However, in doing so you receive more than one submission of other metrics, which may break being able to simply copy/paste things like aggregator.assert_metric(metric_submitted_twice, count=1). In future we should look into how to deduplicate results.
  • Currently, code coverage of e2e tests is not able to be reported alongside regular tests. Until we implement that we should ignore them from affecting percentages with # no cov.

@codecov
Copy link

codecov bot commented Jun 10, 2019

Codecov Report

Merging #3896 into master will decrease coverage by 4.6%.
The diff coverage is 17.64%.

@@            Coverage Diff             @@
##           master    #3896      +/-   ##
==========================================
- Coverage   86.48%   81.88%   -4.61%     
==========================================
  Files         740       96     -644     
  Lines       37835     6204   -31631     
  Branches     4481      731    -3750     
==========================================
- Hits        32723     5080   -27643     
+ Misses       3892      962    -2930     
+ Partials     1220      162    -1058

Copy link
Contributor

@therve therve left a comment

Choose a reason for hiding this comment

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

Looks good, a few questions inside.

.travis.yml Show resolved Hide resolved
# already been spun up so we prevent another invocation
elif e2e_testing(): # no cov
# Since the scope is `session` there should only ever be one definition
fixture_def = request._fixturemanager._arg2fixturedefs[E2E_FIXTURE_NAME][0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch. Is there a way not to do that? That looks super fragile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really tried, but I found absolutely no other way

tests_ran = True

for env in envs:
if clean:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be tempted to not have that clean option. My suggestion would be to do start/test/stop by default. We can keep ddev test --e2e to iterate without setup/teardown of the env, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is if one of us or a contributor forgets that flag then the env would be town down with a random agent still up, leaving it in an inconsistent state

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess... I'm really against using clean though, considering it starts the environment.

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 I understand what's the suggested workflow here.
The idea is to do ddev env test --clean and that will start, test and stop the env?

I don't understand very well @therve suggested workflow or @ofek's concern here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note here: As the ideais not to have to run e2e locally for releases anymore I guess the case of start/test/stop is going to become less frequent for developers than it is now

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's fine. We can always tweak the process later on. I think detecting if the env is running would be a nice improvement.

'config (agent5, agent6, etc.)'
),
)
@click.option('--dev/--prod', help='Whether to use the latest version of a check or what is shipped')
Copy link
Contributor

Choose a reason for hiding this comment

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

which one is the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--prod

'Ex: -e DD_URL=app.datadoghq.com -e DD_API_KEY=123456'
),
)
@click.option('--clean', '-c', is_flag=True, help='Execute setup and tear down actions')
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe sth like --new-env clean has already a lot of meaning from build scripts and it sounds confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for --new-env.

tests_ran = True

for env in envs:
if clean:
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 I understand what's the suggested workflow here.
The idea is to do ddev env test --clean and that will start, test and stop the env?

I don't understand very well @therve suggested workflow or @ofek's concern here.

@ofek ofek merged commit 6caa96d into master Jun 14, 2019
@ofek ofek deleted the ofek/e2e branch June 14, 2019 17:11
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