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

[Monitoring] Skip tests for cloud #30879

Merged
merged 3 commits into from
Feb 14, 2019

Conversation

chrisronline
Copy link
Contributor

@chrisronline chrisronline commented Feb 12, 2019

Relates to https://github.com/elastic/stack-monitoring/issues/31
Resolves #30707
Resolves #30708
Resolves #30709
Resolves #30713

This PR adds logic to skip certain monitoring tests for cloud. The reasons are explained in each ticket. In the future, we can resolve these properly, but skipping for now will help avoid these unnecessary failures. None of the failures are due to an actual bug in the code.

@chrisronline chrisronline added the Team:Monitoring Stack Monitoring team label Feb 12, 2019
@chrisronline chrisronline self-assigned this Feb 12, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/stack-monitoring

@ycombinator
Copy link
Contributor

@chrisronline I'm okay with the skips here as long as we have an issue to re-enable these tests with the proper resolution. Could you create such an issue (if necessary) and link to it from a TODO comment above each this.tags(['skipCloud']); line introduced in this PR?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor

ycombinator commented Feb 13, 2019

@chrisronline In your TODO comments you linked to https://github.com/elastic/stack-monitoring/issues/31. But according to this PR's description, merging this PR will resolve https://github.com/elastic/stack-monitoring/issues/31. This causes two problems, IMO:

  1. Fast forward say 6 months when we see the TODOs in the tests code. We'll follow the link, see a closed issue and think the TODO comments are stale instead of thinking that there's actual work to be done.

  2. https://github.com/elastic/stack-monitoring/issues/31 would be closed (by this PR) so we have no open issue tracking the actual pending work of re-enabling these skipped tests with proper resolutions.

I'd prefer creating a new issue (separate from https://github.com/elastic/stack-monitoring/issues/31) that's clearly about re-enabling the tests with proper resolution and linking to that issue from the TODO comments. But if you think this is unnecessary and we won't lose track of these skipped tests, then this PR LGTM.

@chrisronline
Copy link
Contributor Author

I'd prefer to keep a single ticket. I'm going to update the ticket to include this better.

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@chrisronline
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline chrisronline merged commit 13e5331 into elastic:master Feb 14, 2019
@chrisronline chrisronline deleted the monitoring/skip_cloud_tests branch February 14, 2019 04:19
chrisronline added a commit to chrisronline/kibana that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
chrisronline added a commit to chrisronline/kibana that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
chrisronline added a commit to chrisronline/kibana that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
chrisronline added a commit that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
chrisronline added a commit that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
chrisronline added a commit that referenced this pull request Feb 14, 2019
* Skip tests for cloud

* Add TODOs
@chrisronline
Copy link
Contributor Author

Backport:

7.x: b327085
7.0: 85ce5e6
6.7: b2fbb33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment