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

Add ddev test option to verify support of new metrics #6141

Merged
merged 4 commits into from
Mar 25, 2020

Conversation

mgarabed
Copy link
Member

What does this PR do?

Adds a new option to ddev test called --check-metrics.

When the flag is present, only tests marked by @pytest.mark.check_metrics will run.

Motivation

The original motivation for these tests were to ensure that we kept ahead of new developments for the integrations. However, it's turned out that development on these areas progresses a little more frequently than expected, and running these tests as part of our usual CI runs can make things noisy and require fixes more quickly than really needed.

Adding this option will move the checks out of regular CI execution, and a separate PR will then be setup to schedule them for periodic execution so we still maintain the insight when new metrics get added and can schedule a fix accordingly.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@codecov
Copy link

codecov bot commented Mar 24, 2020

ChristineTChen
ChristineTChen previously approved these changes Mar 24, 2020
Copy link
Contributor

@ChristineTChen ChristineTChen left a comment

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Nice! WDYTA calling the marker metric_validation?

Comment on lines 247 to 254
def pytest_collection_modifyitems(config, items):
if config.getoption("--run-check-metrics"):
# --run-check-metrics given in cli: do not skip slow tests
return
skip_check_metrics = pytest.mark.skip(reason="need --run-check-metrics option to run")
for item in items:
if "check_metrics" in item.keywords:
item.add_marker(skip_check_metrics)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explain what this does in comments?

@florimondmanca
Copy link
Contributor

florimondmanca commented Mar 25, 2020

Great idea!

I agree the naming should be a little more precise — eg the word "latest" should be present. (IMO metrics_check and metric_validation make me think of eg "this test verifies check yields metrics in line with metadata.csv", not that it tries to see if we're up to date with the latest available metrics.)

WDYT of latest_metrics?

[pytest]
marks =
    latest_metrics: Mark test as checking for latest available metrics
@pytest.mark.latest_metrics
def check_metrics_are_up_to_date():
    ...
$ ddev test --latest-metrics

@mgarabed
Copy link
Member Author

Agreed on the naming, I struggled with that one and wasn't totally happy with the one I picked either.

I like latest-metrics - we have a bunch of validation checks already, and while this performs a similar "validation" to the ddev sub-commands if we used the same name for the tests it could easy to confuse what's actually happening.

@mgarabed
Copy link
Member Author

Also, pro-tip which was helpful here, you can get explicit skip reasons by passing -rs to pytest, so for our ddev command:

ddev test -k test_latest_metrics_supported -pa -rs clickhouse:py38-latest

@ofek ofek changed the title Add check-metrics option to ddev test Add ddev test option to verify support of new metrics Mar 25, 2020
Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>
@mgarabed mgarabed merged commit c5501c0 into master Mar 25, 2020
@mgarabed mgarabed deleted the mgarabed/metrics-check-test branch March 25, 2020 22:13
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.

None yet

4 participants