Add ability to skip tests by ycombinator · Pull Request #221 · elastic/elastic-package

ycombinator · 2021-01-15T05:14:42Z

This PR adds the ability for package developers to skip tests.

TODO

Implement skipping for asset loading test runner. Depends on Asset loading test runner #168.
Extract common skip struct from various test runners.
Add some skipped tests in test packages.

Related: #218.

elasticmachine · 2021-01-15T05:26:09Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #221 updated
- Start Time: 2021-01-29T19:46:49.073+0000
Duration: 15 min 16 sec
Commit: b195c06

Test stats 🧪

Test	Results
Failed	0
Passed	290
Skipped	1
Total	291

ycombinator · 2021-01-27T00:14:45Z

I need to figure out why CI is not showing the apache/error system test as skipped.

The human formatter appears to be working as expected:

$ elastic-package test system --data-streams error
Run system tests for the package
2021/01/26 16:16:00  WARN skipping system test for apache/error: testing skip (details: )
--- Test results for package: apache - START ---
╭─────────┬─────────────┬───────────┬───────────┬─────────┬──────────────╮
│ PACKAGE │ DATA STREAM │ TEST TYPE │ TEST NAME │ RESULT  │ TIME ELAPSED │
├─────────┼─────────────┼───────────┼───────────┼─────────┼──────────────┤
│ apache  │ error       │ system    │ default   │ SKIPPED │        579ns │
╰─────────┴─────────────┴───────────┴───────────┴─────────┴──────────────╯
--- Test results for package: apache - END   ---
Done

The xUnit formatter also appears to be correct, but I will probably need to play around with the attributes/elements and their values to get skipped tests working properly in CI.

$ elastic-package test system --data-streams error --report-format xUnit
Run system tests for the package
2021/01/26 16:16:35  WARN skipping system test for apache/error: testing skip (details: )
--- Test results for package: apache - START ---
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="system" tests="1" skipped="1">
    <!--test suite for system tests-->
    <testcase name="system test: default" result="Skip" classname="apache.error" time="2.77e-07"></testcase>
  </testsuite>
</testsuites>
--- Test results for package: apache - END   ---
Done

mtojek · 2021-01-28T09:16:13Z

The xUnit formatter also appears to be correct, but I will probably need to play around with the attributes/elements and their values to get skipped tests working properly in CI.

If you prefer to play with it in follow up (bugfix), I'm good with it!

mtojek

Minors only and a general comment about the Skip section, but nothing blocking I believe.

internal/testrunner/runners/pipeline/test_config.go

internal/testrunner/testrunner.go

internal/testrunner/runners/system/runner.go

mtojek · 2021-01-28T09:49:45Z

internal/testrunner/runners/pipeline/runner.go

 		}
 		tr.Name = tc.name

+		if tc.config.Skip != nil {


I think I have a general concern, but I'm sure if it can be solved. I'm worried that one day we'll add a new test runner, in which we'll forget to add the "Skip" config section. Maybe we should limit this to minimum. Do we need a skip check for assets tests (same for install)?

This is a good point. Maybe this goes well with your other suggestion of a commonTestConfig. Let me try something and update this PR.

In 4e25215, I introduced a SkippableConfig struct that could be embedded in any test config structs that allow skipping. This doesn't necessarily address the concern you are bringing up (a future test runner forgetting to include the skip config section) but I think it can help make it a bit easier to add a skip config section to a future test runner. I think to truly address your concern a larger refactor might be needed which would allow a higher-level generic test runner of some sort to handle skipping before delegating the actual running of a test to specific types of test runners.

I think to truly address your concern a larger refactor might be needed which would allow a higher-level generic test runner of some sort to handle skipping before delegating the actual running of a test to specific types of test runners.

... and we may think about such refactoring exercise soon, as we have a better clarity of what do we expect from test runners and what kind of tests we need. Definitely we can postpone it at the moment.

Agreed. Added #238 to track the refactoring.

ycombinator · 2021-01-29T02:35:38Z

@mtojek This PR is now failing CI with an unrelated error: https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Felastic-package/detail/PR-221/20/pipeline#step-122-log-693.

The same / similar error is occurring on master as well: https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Felastic-package/detail/master/22/pipeline/#step-108-log-544

I would suggest that you review this PR for the latest changes but let's not merge it even when it passes review. I think we need to take care of the error on master separately first, then we can rebase this PR on master and see if it passes CI as well.

ycombinator · 2021-01-29T02:39:58Z

I believe the CI error I mentioned in my previous comment will be resolved once we have a new Kibana 7.11 Docker image. Related: elastic/kibana#89644.

ycombinator · 2021-01-29T12:58:47Z

jenkins, run the tests please

mtojek · 2021-01-29T13:01:52Z

internal/testrunner/runners/pipeline/runner.go

 		}
 		tr.Name = tc.name

+		if tc.config.Skip != nil {


I think to truly address your concern a larger refactor might be needed which would allow a higher-level generic test runner of some sort to handle skipping before delegating the actual running of a test to specific types of test runners.

... and we may think about such refactoring exercise soon, as we have a better clarity of what do we expect from test runners and what kind of tests we need. Definitely we can postpone it at the moment.

mtojek · 2021-01-29T13:03:13Z

test/packages/apache/data_stream/error/_dev/test/system/test-default-config.yml

+  link: https://github.com/elastic/integrations/issues/123456789
 vars: ~
 data_stream:
  vars:


I assume that this test will fail if it runs, that's why it's skipped = PASS

This used to be a test that used to pass before I added the skip. I think you are suggesting that I should make it fail, so we have a way to know something is not working correctly if the skip logic in the test runner breaks? I like that idea, and can implement it — just want to make sure that's what you are suggesting here first. 🙂

Yes, that's point :) I think the quick way is to introduce some faulty variables? Don't have to implement anything special. But I wouldn't say it's blocking comment. For me this PR is ready to be merged.

Thanks for confirming, I will make this change now. Should be fairly quick to test.

I'm going to do a series of 3 commits to test this change:

A commit that comments out the skip section, effectively re-enabling the apache.error system test. With this commit the test should pass: 322c835.

A commit that breaks the test. With this commit the test should fail: cec86cf.

A commit that adds back the skip section. With this commit the test should be skipped: 0fbf9ad.

mtojek

LGTM!

ycombinator mentioned this pull request Jan 26, 2021

Allow tests to be disabled #218

Closed

2 tasks

ycombinator force-pushed the test-skip branch from 8543e24 to 5bcde55 Compare January 26, 2021 01:06

ycombinator added 20 commits January 27, 2021 14:23

Adding skipped to reporters

79e2647

Adding skipped to pipeline test runner

255cdbc

Adding skipped to system test runner

514e05e

Adding skipped field to test result

be3c26c

Add skip configuration for asset loading tests

35aa187

Refactoring: extracting result composer for reuse in multiple packages

078963c

Use result composer in asset loading test runner

71e99e3

Adding godoc

706263f

Refactoring: extract skip config struct

a994a4d

Adding license header + struct godoc

a9d89f8

Account for no test config

1513b04

Fix error message

fb6b90e

Adding skip section to test package

9feae09

Add warning logs for skipped tests

5ba06fb

Include test result

c409d93

Stringify link

feadfbe

Using result

3998403

Fixing format

1c9dc9a

Make skipped optional

91b97ed

Add skip reason and link

9e66712

ycombinator force-pushed the test-skip branch from 6795d06 to 9e66712 Compare January 27, 2021 22:24

ycombinator marked this pull request as ready for review January 27, 2021 22:24

ycombinator requested a review from mtojek January 27, 2021 22:24

mtojek approved these changes Jan 28, 2021

View reviewed changes

Introducing SkippableConfig

4e25215

Introducing constructor for ResultComposer

d93be29

ycombinator requested a review from mtojek January 29, 2021 02:40

mtojek approved these changes Jan 29, 2021

View reviewed changes

ycombinator added 3 commits January 29, 2021 10:13

Temporarily commenting out skip so test runs again

322c835

Making test fail with bad path

cec86cf

Putting back skip section

0fbf9ad

ycombinator requested a review from mtojek January 29, 2021 19:29

Merge branch 'master' into test-skip

b195c06

mtojek approved these changes Feb 1, 2021

View reviewed changes

ycombinator merged commit 86360ba into elastic:master Feb 1, 2021

ycombinator deleted the test-skip branch February 1, 2021 15:01

Conversation

ycombinator commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

elasticmachine commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

ycombinator commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtojek commented Jan 28, 2021

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Jan 29, 2021

Uh oh!

ycombinator commented Jan 29, 2021

Uh oh!

ycombinator commented Jan 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ycombinator Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ycombinator commented Jan 15, 2021 •

edited

Loading

elasticmachine commented Jan 15, 2021 •

edited

Loading

ycombinator commented Jan 27, 2021 •

edited

Loading

ycombinator Jan 29, 2021 •

edited

Loading

ycombinator Jan 29, 2021 •

edited

Loading