Asset loading test runner #168

ycombinator · 2020-11-10T09:02:56Z

This PR implements a new type of test runner: asset. This test runner tests whether a package's assets (ES index templates, Kibana dashboards, etc.) are loaded up as expected.

Sample usage

$ elastic-package test asset
Run asset tests for the package
--- Test results for package: apache - START ---
╭─────────┬─────────────┬───────────┬────────────────────────────────────────────────────────────────┬────────┬──────────────╮
│ PACKAGE │ DATA STREAM │ TEST TYPE │ TEST NAME                                                      │ RESULT │ TIME ELAPSED │
├─────────┼─────────────┼───────────┼────────────────────────────────────────────────────────────────┼────────┼──────────────┤
│ apache  │             │ asset     │ dashboard apache-Logs-Apache-Dashboard is loaded               │ PASS   │ 6.101128287s │
│ apache  │             │ asset     │ dashboard apache-Metrics-Apache-HTTPD-server-status is loaded  │ PASS   │        927ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-CPU is loaded                       │ PASS   │        620ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Hostname-list is loaded             │ PASS   │        512ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Load1-slash-5-slash-15 is loaded    │ PASS   │        526ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Scoreboard is loaded                │ PASS   │        430ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Total-accesses-and-kbytes is loaded │ PASS   │        424ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Uptime is loaded                    │ PASS   │        416ns │
│ apache  │             │ asset     │ visualization apache-HTTPD-Workers is loaded                   │ PASS   │        403ns │
│ apache  │             │ asset     │ visualization apache-access-unique-IPs-map is loaded           │ PASS   │        404ns │
│ apache  │             │ asset     │ visualization apache-browsers is loaded                        │ PASS   │        413ns │
│ apache  │             │ asset     │ visualization apache-error-logs-over-time is loaded            │ PASS   │        410ns │
│ apache  │             │ asset     │ visualization apache-operating-systems is loaded               │ PASS   │        407ns │
│ apache  │             │ asset     │ visualization apache-response-codes-of-top-URLs is loaded      │ PASS   │        439ns │
│ apache  │             │ asset     │ visualization apache-response-codes-over-time is loaded        │ PASS   │        453ns │
│ apache  │             │ asset     │ search apache-HTTPD is loaded                                  │ PASS   │        598ns │
│ apache  │             │ asset     │ search apache-access-logs is loaded                            │ PASS   │        420ns │
│ apache  │             │ asset     │ search apache-errors-log is loaded                             │ PASS   │        514ns │
│ apache  │ access      │ asset     │ index_template logs-apache.access is loaded                    │ PASS   │        490ns │
│ apache  │ access      │ asset     │ ingest_pipeline logs-apache.access-0.2.7 is loaded             │ PASS   │        462ns │
│ apache  │ error       │ asset     │ index_template logs-apache.error is loaded                     │ PASS   │        405ns │
│ apache  │ error       │ asset     │ ingest_pipeline logs-apache.error-0.2.7 is loaded              │ PASS   │        434ns │
│ apache  │ status      │ asset     │ index_template metrics-apache.status is loaded                 │ PASS   │        440ns │
╰─────────┴─────────────┴───────────┴────────────────────────────────────────────────────────────────┴────────┴──────────────╯
--- Test results for package: apache - END   ---
Done

TODO

Resolves #115.

elasticmachine · 2020-11-10T09:07:19Z

💚 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 #168 updated
- Start Time: 2021-01-15T19:34:03.452+0000
Duration: 11 min 19 sec
Commit: 133e50e

Test stats 🧪

Test	Results
Failed	0
Passed	67
Skipped	0
Total	67

ycombinator · 2020-11-17T09:22:45Z

@mtojek This PR is ready for review, when you get a chance.

Before this PR we had 2 test runners, both of which operated at the data stream level and needed test configuration. This PR introduces a test runner that operates at the package level and also doesn't need test configuration. As a result, the code for the test runner framework is now a bit of a mess IMO. I would like to clean this up but in a follow up PR. WDYT?

internal/packages/assets.go

internal/kibana/ingestmanager/client_packages.go

mtojek · 2020-11-17T10:12:25Z

internal/kibana/ingestmanager/client_packages.go

+
+// RemovePackage removes the given package from Fleet.
+func (c *Client) RemovePackage(pkg packages.PackageManifest) ([]packages.Asset, error) {
+	return managePackage(pkg, "remove", c.delete)


I have a gut feeling that clients in ingestmanager are too much complex. Not sure if we need as many layers to process a single request. Maybe not depending on common abstraction would be simpler?

Please take a look at https://github.com/elastic/elastic-package/blob/master/internal/kibana/saved_objects.go . In this case I need to implement paging and the exact code responsible for calls to Kibana is wrapped up in ca. 20 LOCs: https://github.com/elastic/elastic-package/blob/master/internal/kibana/saved_objects.go#L95-L118

I'm happy to discuss it!

mtojek · 2020-11-17T10:19:15Z

internal/packages/assets.go

+	}
+	assets = append(assets, a...)
+
+	a, err = loadFileBasedAssets(kibanaAssetsFolderPath, AssetTypeKibanaMap)


Thinking loud: shouldn't elastic-package discover available types from the package directory? In case we add a new type the command will still operate (open-closed principle).

internal/packages/assets.go

internal/testrunner/runners/asset/runner.go

mtojek · 2020-11-17T10:38:53Z

test/packages/apache/manifest.yml

@@ -1,7 +1,7 @@
 format_version: 1.0.0
 name: apache
 title: Apache
-version: 0.1.4
+version: 0.2.7


Is this change on purpose?

Yes, I had to make it because the package registry image loaded up by the tests installs Apache version 0.2.6. So, in order for the asset test runner to be able to install the local apache package (i.e. the one being tested), I had to ensure that the local apache package's version was greater than the one in the registry. Otherwise the Install Package API would not allow the local package to be loaded up.

I admit that this is not a very maintainable change. Every time the package registry image is refreshed with a newer version of apache, we will need to bump up the version here as well. Maybe we should just set it here to something like 999.999.999?

Oh, I see :) In this case you can set it to sth weird like 999. (at least the major). Nice catch!

mtojek · 2020-11-17T10:40:36Z

scripts/test-check-packages.sh

+for d in test/packages/*/; do
+  (
+    cd $d
+    elastic-package build -v


Could you please double-check if the check command doesn't include build step?

internal/testrunner/testrunner.go

ycombinator · 2020-11-19T18:18:51Z

TODO: consider implementing #175 (comment) as part of this PR.

mtojek

@ycombinator please let me know whether I can take another look at this PR.

ycombinator · 2021-01-04T15:21:53Z

please let me know whether I can take another look at this PR.

Yeah, I'm still iterating on it. I'll re-request your review when it's ready for you to take a look. Thanks!

ycombinator · 2021-01-14T05:37:58Z

I see that CI is failing. I will look into this tomorrow.

mtojek

The implementation looks good to me.

Let's move forward with this PR. Did you take a chance to run it against real integrations?

nit: just rebase the PR against master branch

ycombinator · 2021-01-14T23:51:00Z

Seeing this error now:

$ elastic-package test -v --report-format xUnit --report-output file --defer-cleanup 1s
elastic-package has been installed.
2021/01/14 15:49:34 DEBUG Enable verbose logging
Run test suite for the package
Run asset tests for the package
2021/01/14 15:49:34 DEBUG installing package...
2021/01/14 15:49:34 DEBUG POST http://127.0.0.1:5601/api/fleet/epm/packages/apache-999.999.999
2021/01/14 15:49:34 DEBUG
2021/01/14 15:49:43 DEBUG waiting for 1s before tearing down...
Run pipeline tests for the package
Error: error parsing --data-streams flag: flag accessed but not defined: data-streams

Will look into it tomorrow.

ycombinator · 2021-01-15T04:41:37Z

Hi @mtojek, I made a few small changes since your last review to make CI pass. Could you please take another look when you get a chance? Thanks!

mtojek

Thank you for the heads up! I left some minor comments, but wouldn't like to block your PR from merging. These ones can be addressed in follow up PRs (up to you).

I'm thrilled to see this running in the Integrations. Nice job!

mtojek · 2021-01-15T08:20:23Z

internal/packages/assets.go

+	AssetTypeKibanaSavedSearch   AssetType = "search"
+	AssetTypeKibanaVisualization AssetType = "visualization"
+	AssetTypeKibanaDashboard     AssetType = "dashboard"
+	AssetTypeKibanaMap           AssetType = "map"


Addressed in 133e50e.

mtojek · 2021-01-15T08:30:13Z

internal/testrunner/runners/system/runner.go

@@ -51,17 +51,6 @@ type runner struct {
 	wipeDataStreamHandler   func() error
 }

-type stackSettings struct {


Is the stackSettings (old and new) structure used anywhere? It looks like a leftover after refactoring. All stack settings like host, login, password don't need to be exposed beyond ES/Kibana clients.

Good catch. I've removed all stack settings code in cef5f22. So now the ES/Kibana clients directly call os.Getenv in their constructors to get the stack settings.

internal/packages/packages.go

mtojek · 2021-01-15T08:38:18Z

internal/packages/packages.go

@@ -223,5 +237,5 @@ func isDataStreamManifest(path string) (bool, error) {
 	if err != nil {
 		return false, errors.Wrapf(err, "reading package manifest failed (path: %s)", path)
 	}
-	return m.Title != "" && (m.Type == "logs" || m.Type == "metrics"), nil
+	return m.Title != "" && (m.Type == dataStreamTypeLogs || m.Type == dataStreamTypeMetrics), nil


nit: Actually this function isn't perfect. If there is another dataset type, it will simply fail (OCP broken). I wonder if there is a different way to recognize the DataStream. If you find any, feel free to adjust the implementation.

Yeah, it's not perfect but I think this check here might have some good side-effects. In the asset test runner we check if we are dealing with a logs data stream and, if so, check that ingest pipeline assets have also been loaded. Similarly, whenever a new data stream type is added in the future we might need to have special handling for it somewhere in elastic-package. So being strict here about which data stream types elastic-package knows how to handle may be a good thing.

Case in point: I was checking the data stream manifest spec to see if we have defined the data stream type field as an enum and we have. However, while doing this I noticed that there is a data stream type we have not yet considered in elastic-package: traces. It was added for APM in elastic/package-spec#78. I'm guessing the APM team is not using elastic-package yet so they are not running into any issues with this type not being supported. I've created #223 to track adding support for this type in elastic-package.

ycombinator · 2021-01-15T19:51:42Z

@mtojek I've addressed all your feedback. Please re-review when you get a chance. If the PR looks good to you, please merge it on my behalf as I'll be away from work for the next week. Otherwise I will address any further feedback when I'm back. Thanks!

ycombinator force-pushed the test-runner-asset branch from 06e1d5b to 4a0b952 Compare November 17, 2020 05:57

ycombinator marked this pull request as ready for review November 17, 2020 09:18

ycombinator force-pushed the test-runner-asset branch from 41fd971 to 47ec7e2 Compare November 17, 2020 09:19

ycombinator requested a review from mtojek November 17, 2020 09:22

ycombinator changed the title ~~[WIP] Asset loading test runner~~ Asset loading test runner Nov 17, 2020

mtojek reviewed Nov 17, 2020

View reviewed changes

mtojek mentioned this pull request Nov 19, 2020

Defer cleanup flag #175

Merged

ycombinator mentioned this pull request Nov 23, 2020

Refactoring test runner #180

Merged

ycombinator force-pushed the test-runner-asset branch from 5a70284 to 3820375 Compare November 24, 2020 10:45

ycombinator force-pushed the test-runner-asset branch from 3820375 to 380bcb0 Compare December 9, 2020 07:55

ycombinator force-pushed the test-runner-asset branch from 380bcb0 to f13bba7 Compare December 29, 2020 19:31

ycombinator mentioned this pull request Dec 30, 2020

Merging ingestmanager client into generic kibana client #210

Merged

mtojek reviewed Jan 4, 2021

View reviewed changes

ycombinator force-pushed the test-runner-asset branch from 23fb729 to 36a28f5 Compare January 6, 2021 01:08

ycombinator force-pushed the test-runner-asset branch from 36a28f5 to 693a1f8 Compare January 14, 2021 02:20

ycombinator requested a review from mtojek January 14, 2021 02:44

mtojek approved these changes Jan 14, 2021

View reviewed changes

ycombinator added 8 commits January 14, 2021 13:51

Refactoring: extracting common code

05c160c

Adding skeleton for asset loading test runner

ebf3a50

Refactoring: defining and implementing TestRunner interface

27e1a08

More fleshing out

2dfb9f8

More fleshing out

e13e739

Adding HOWTO

eefad58

Refactoring: make ingest pipeline name method of DataStreamManifest

459df7e

Adding assertions

af707f3

ycombinator added 6 commits January 14, 2021 13:51

Adding license headers

f482e49

Using nil slices

778b9cf

Move settings to stack package

b3f752b

Refactoring: merging packages API into Kibana client

8af8e4b

Use multierror to collect all errors

5aecd27

Removing IsConfigRequired

4ec725f

ycombinator force-pushed the test-runner-asset branch from 0e527da to 4ec725f Compare January 14, 2021 22:19

ycombinator added 6 commits January 14, 2021 14:23

Bumping up apache version to something very large

91fd2c8

Adding comment for large version

c0014db

Fixing paths

dfd0503

Check errors length

93cff73

Fixing typo

5d14f10

Initialize OLDPWD

4760e8c

ycombinator added 3 commits January 14, 2021 19:51

Check for existence of flag before trying to parse it

1bfb2c0

Adding comment to explain check

3d11f91

Setting large versions for all test packages

010d452

ycombinator requested a review from mtojek January 15, 2021 04:41

ycombinator mentioned this pull request Jan 15, 2021

Add ability to skip tests #221

Merged

3 tasks

mtojek approved these changes Jan 15, 2021

View reviewed changes

Removing vestigial stack settings code

cef5f22

ycombinator force-pushed the test-runner-asset branch from e4feb39 to cef5f22 Compare January 15, 2021 19:24

Adding lens asset

133e50e

ycombinator requested a review from mtojek January 15, 2021 19:50

mtojek approved these changes Jan 18, 2021

View reviewed changes

mtojek merged commit 2e65b64 into elastic:master Jan 18, 2021

ycombinator deleted the test-runner-asset branch January 25, 2021 18:13

ycombinator mentioned this pull request Feb 23, 2021

test asset: validate fields in sample event #226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset loading test runner #168

Asset loading test runner #168

ycombinator commented Nov 10, 2020 •

edited

Loading

elasticmachine commented Nov 10, 2020 •

edited

Loading

Build stats

Test stats 🧪

ycombinator commented Nov 17, 2020

mtojek Nov 17, 2020

mtojek Nov 17, 2020

mtojek Nov 17, 2020

ycombinator Jan 14, 2021

mtojek Jan 14, 2021

mtojek Nov 17, 2020

ycombinator commented Nov 19, 2020

mtojek left a comment

ycombinator commented Jan 4, 2021

ycombinator commented Jan 14, 2021

mtojek left a comment •

edited

Loading

ycombinator commented Jan 14, 2021

ycombinator commented Jan 15, 2021

mtojek left a comment

mtojek Jan 15, 2021

ycombinator Jan 15, 2021

mtojek Jan 15, 2021

ycombinator Jan 15, 2021

mtojek Jan 15, 2021

ycombinator Jan 15, 2021 •

edited

Loading

ycombinator commented Jan 15, 2021

Asset loading test runner #168

Asset loading test runner #168

Conversation

ycombinator commented Nov 10, 2020 • edited Loading

Sample usage

TODO

elasticmachine commented Nov 10, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

ycombinator commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented Nov 19, 2020

mtojek left a comment

Choose a reason for hiding this comment

ycombinator commented Jan 4, 2021

ycombinator commented Jan 14, 2021

mtojek left a comment • edited Loading

Choose a reason for hiding this comment

ycombinator commented Jan 14, 2021

ycombinator commented Jan 15, 2021

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

ycombinator commented Jan 15, 2021

ycombinator commented Nov 10, 2020 •

edited

Loading

elasticmachine commented Nov 10, 2020 •

edited

Loading

mtojek left a comment •

edited

Loading

ycombinator Jan 15, 2021 •

edited

Loading