-
Notifications
You must be signed in to change notification settings - Fork 139
Skip by default compareResults in pipeline tests for serverless #1521
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,7 @@ import ( | |||||
|
|
||||||
| "github.com/elastic/elastic-package/internal/common" | ||||||
| "github.com/elastic/elastic-package/internal/elasticsearch/ingest" | ||||||
| "github.com/elastic/elastic-package/internal/environment" | ||||||
| "github.com/elastic/elastic-package/internal/fields" | ||||||
| "github.com/elastic/elastic-package/internal/logger" | ||||||
| "github.com/elastic/elastic-package/internal/multierror" | ||||||
|
|
@@ -33,9 +34,15 @@ const ( | |||||
| TestType testrunner.TestType = "pipeline" | ||||||
| ) | ||||||
|
|
||||||
| var ( | ||||||
| serverlessEnableCompareResults = environment.WithElasticPackagePrefix("SERVERLESS_PIPELINE_TEST_ENABLE_COMPARE_RESULTS") | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking now that maybe we should leave this enabled by default, so when running locally, by default we see the errors. And we can disable in the CI jobs where we want to use this. WDYT?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, agreed |
||||||
| ) | ||||||
|
|
||||||
| type runner struct { | ||||||
| options testrunner.TestOptions | ||||||
| pipelines []ingest.Pipeline | ||||||
|
|
||||||
| runCompareResults bool | ||||||
| } | ||||||
|
|
||||||
| type IngestPipelineReroute struct { | ||||||
|
|
@@ -61,6 +68,22 @@ func (r *runner) String() string { | |||||
| // Run runs the pipeline tests defined under the given folder | ||||||
| func (r *runner) Run(options testrunner.TestOptions) ([]testrunner.TestResult, error) { | ||||||
| r.options = options | ||||||
|
|
||||||
| stackConfig, err := stack.LoadConfig(r.options.Profile) | ||||||
| if err != nil { | ||||||
| return nil, err | ||||||
| } | ||||||
|
|
||||||
| r.runCompareResults = true | ||||||
| if stackConfig.Provider == stack.ProviderServerless { | ||||||
| r.runCompareResults = false | ||||||
|
|
||||||
| v, ok := os.LookupEnv(serverlessEnableCompareResults) | ||||||
| if ok && strings.ToLower(v) != "false" { | ||||||
| r.runCompareResults = true | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return r.run() | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -303,20 +326,14 @@ func (r *runner) verifyResults(testCaseFile string, config *testConfig, result * | |||||
| } | ||||||
|
|
||||||
| // TODO: currently GeoIP related fields are being removed when the serverless provider is used. | ||||||
| stackConfig, err := stack.LoadConfig(r.options.Profile) | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
| skipGeoIP := false | ||||||
| if stackConfig.Provider == stack.ProviderServerless { | ||||||
| skipGeoIP = true | ||||||
| } | ||||||
| err = compareResults(testCasePath, config, result, skipGeoIP, *specVersion) | ||||||
| if _, ok := err.(testrunner.ErrTestCaseFailed); ok { | ||||||
| return err | ||||||
| } | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("comparing test results failed: %w", err) | ||||||
| if r.runCompareResults { | ||||||
| err = compareResults(testCasePath, config, result, *specVersion) | ||||||
| if _, ok := err.(testrunner.ErrTestCaseFailed); ok { | ||||||
| return err | ||||||
| } | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("comparing test results failed: %w", err) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| result = stripEmptyTestResults(result) | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,28 +24,6 @@ import ( | |
|
|
||
| const expectedTestResultSuffix = "-expected.json" | ||
|
|
||
| var geoIPKeys = []string{ | ||
| "as", | ||
| "geo", | ||
| "client.as", | ||
| "client.geo", | ||
| "destination.as", | ||
| "destination.geo", | ||
| "host.geo", // not defined host.as in ECS | ||
| "observer.geo", // not defined observer.as in ECS | ||
| "server.as", | ||
| "server.geo", | ||
| "source.as", | ||
| "source.geo", | ||
| "threat.enrichments.indicateor.as", | ||
| "threat.enrichments.indicateor.geo", | ||
| "threat.indicateor.as", | ||
| "threat.indicateor.geo", | ||
| // packages using geo fields in nested objects | ||
| "netskope.alerts.user.geo", | ||
| "netskope.events.user.geo", | ||
| } | ||
|
|
||
|
Comment on lines
-27
to
-48
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed since in this PR it is removed the comparison of documents with the samples. |
||
| type testResult struct { | ||
| events []json.RawMessage | ||
| } | ||
|
|
@@ -69,8 +47,8 @@ func writeTestResult(testCasePath string, result *testResult, specVersion semver | |
| return nil | ||
| } | ||
|
|
||
| func compareResults(testCasePath string, config *testConfig, result *testResult, skipGeoip bool, specVersion semver.Version) error { | ||
| resultsWithoutDynamicFields, err := adjustTestResult(result, config, skipGeoip) | ||
| func compareResults(testCasePath string, config *testConfig, result *testResult, specVersion semver.Version) error { | ||
| resultsWithoutDynamicFields, err := adjustTestResult(result, config) | ||
| if err != nil { | ||
| return fmt.Errorf("can't adjust test results: %w", err) | ||
| } | ||
|
|
@@ -80,7 +58,7 @@ func compareResults(testCasePath string, config *testConfig, result *testResult, | |
| return fmt.Errorf("marshalling actual test results failed: %w", err) | ||
| } | ||
|
|
||
| expectedResults, err := readExpectedTestResult(testCasePath, config, skipGeoip) | ||
| expectedResults, err := readExpectedTestResult(testCasePath, config) | ||
| if err != nil { | ||
| return fmt.Errorf("reading expected test result failed: %w", err) | ||
| } | ||
|
|
@@ -161,7 +139,7 @@ func diffJson(want, got []byte, specVersion semver.Version) (string, error) { | |
| return buf.String(), err | ||
| } | ||
|
|
||
| func readExpectedTestResult(testCasePath string, config *testConfig, skipGeoIP bool) (*testResult, error) { | ||
| func readExpectedTestResult(testCasePath string, config *testConfig) (*testResult, error) { | ||
| testCaseDir := filepath.Dir(testCasePath) | ||
| testCaseFile := filepath.Base(testCasePath) | ||
|
|
||
|
|
@@ -176,15 +154,15 @@ func readExpectedTestResult(testCasePath string, config *testConfig, skipGeoIP b | |
| return nil, fmt.Errorf("unmarshalling expected test result failed: %w", err) | ||
| } | ||
|
|
||
| adjusted, err := adjustTestResult(u, config, skipGeoIP) | ||
| adjusted, err := adjustTestResult(u, config) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("adjusting test result failed: %w", err) | ||
| } | ||
| return adjusted, nil | ||
| } | ||
|
|
||
| func adjustTestResult(result *testResult, config *testConfig, skipGeoIP bool) (*testResult, error) { | ||
| if !skipGeoIP && (config == nil || config.DynamicFields == nil) { | ||
| func adjustTestResult(result *testResult, config *testConfig) (*testResult, error) { | ||
| if config == nil || config.DynamicFields == nil { | ||
| return result, nil | ||
| } | ||
| var stripped testResult | ||
|
|
@@ -210,15 +188,6 @@ func adjustTestResult(result *testResult, config *testConfig, skipGeoIP bool) (* | |
| } | ||
| } | ||
|
|
||
| if skipGeoIP { | ||
| for _, key := range geoIPKeys { | ||
| err := m.Delete(key) | ||
| if err != nil && err != common.ErrKeyNotFound { | ||
| return nil, fmt.Errorf("can't remove geoIP field: %w", err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| b, err := json.Marshal(&m) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("can't marshal event: %w", err) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add somewhere a TODO comment mentioning that this is a temporary workaround till we have something better for deterministic geoip in serverless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the TODO message here: 6b35523