Enable Elasticsearch functional tests for SQL advanced visibility #3923

alexshtin · 2023-02-09T07:13:19Z

What changed?

Enable Elasticsearch functional tests for SQL advanced visibility. With this PR Buildkite runs advanced visibility tests (former Elasticsearch tests) for mysql8, postrges12, and sqlite SQL drivers. Containers which use these drivers doesn't depended on Elasticsearch anymore.
For other drivers it runs same tests using Elasticsearch.
esintegration build tag is removed.

Why?
To cover SQL advanced visibility with existing tests.

How did you test it?
Run them.

Potential risks
No risks.

Is hotfix candidate?
No.

dnr · 2023-02-10T01:37:00Z

tests/namespace_delete_test.go

+		s.Require().NoError(err)
+	} else {
+		var err error
+		clusterConfig, err = GetTestClusterConfig("testdata/integration_namespace_cluster.yaml")


integration_namespace_cluster is a weird name and I have no idea what it means. is there a more descriptive one?

Ok, I cleaned it up. Removed scheduler specific configs too.

dnr · 2023-02-10T01:44:01Z

tests/xdc/advanced_visibility_test.go

+	saListRequest = &workflowservice.ListWorkflowExecutionsRequest{
+		Namespace: namespace,
+		PageSize:  int32(2),
+		Query:     fmt.Sprintf(`WorkflowId = "%s" and %s = "another string"`, id, s.testSearchAttributeKey),


do we have a programmatic query builder yet, or can we factor one out of the new visibility? code like this is kind of scary, even though it's in tests

What's wrong with it? It is functional test. It supposed to emulate user input.

this is fine, but there are a few instances in non-test code now

That's on me. I'll work on making those parts better.

MichaelSnowden · 2023-02-10T17:20:51Z

.golangci.yml

@@ -82,3 +82,9 @@ linters-settings:
      - name: unhandled-error
        arguments:
          - "fmt.Printf"
+issues:
+  exclude-rules:
+    - path: tests\/.+\.go


This will capture tests with tests somewhere in the path, but it won't capture the vast majority of _test.go unit tests that we have. Is that intended?

I want this to get applied to functional tests in tests dir only. But you are right, it will capture others too. I need to add prefix it with ^.

rodrigozhou · 2023-02-10T18:26:02Z

Makefile

+	@go test $(FUNCTIONAL_TEST_ROOT) -timeout=$(TEST_TIMEOUT) $(TEST_TAG) -race -persistenceType=$(PERSISTENCE_TYPE) -persistenceDriver=$(PERSISTENCE_DRIVER) | tee -a test.log
+	@go test $(FUNCTIONAL_TEST_NDC_ROOT) -timeout=$(TEST_TIMEOUT) $(TEST_TAG) -race -persistenceType=$(PERSISTENCE_TYPE) -persistenceDriver=$(PERSISTENCE_DRIVER) | tee -a test.log
 # Need to run xdc tests with race detector off because of ringpop bug causing data race issue.
-	@go test $(FUNCTIONAL_TEST_XDC_ROOT) -timeout=$(TEST_TIMEOUT) $(TEST_TAG) | tee -a test.log
+	@go test $(FUNCTIONAL_TEST_XDC_ROOT) -timeout=$(TEST_TIMEOUT) $(TEST_TAG) -persistenceType=$(PERSISTENCE_TYPE) -persistenceDriver=$(PERSISTENCE_DRIVER) | tee -a test.log


nit: we should have a args var to encapsulate all the common arguments instead of duplicating it.

I think we should remove these args completely and just use envs to generate configs and then use these configs for test clusters.

rodrigozhou · 2023-02-10T18:28:30Z

tests/namespace_delete_test.go

+	if TestFlags.PersistenceDriver == mysql.PluginNameV8 ||
+		TestFlags.PersistenceDriver == postgresql.PluginNameV12 ||
+		TestFlags.PersistenceDriver == sqlite.PluginName {


Isn't switch/case easier to read?

Yes, and in all other places we use switch for these constants also.

rodrigozhou · 2023-02-10T18:31:01Z

tests/namespace_delete_test.go

+		var err error
+		clusterConfig, err = GetTestClusterConfig("testdata/integration_test_cluster.yaml")
+		s.Require().NoError(err)


Don't you need to setup the database? Or is it setup somewhere else?

Yes, both DBs are already setup here. ES index also should be created earlier. This needs to be refactored.

rodrigozhou · 2023-02-10T18:37:58Z

tests/xdc/advanced_visibility_test.go

+	if !s.isElasticsearchEnabled() {
+		// When Elasticsearch is enabled, the search attribute aliases are not used.
+		_, err = client1.UpdateNamespace(tests.NewContext(), &workflowservice.UpdateNamespaceRequest{
+			Namespace: namespace,
+			Config: &namespacepb.NamespaceConfig{
+				CustomSearchAttributeAliases: map[string]string{
+					"Bool01":     "CustomBoolField",
+					"Datetime01": "CustomDatetimeField",
+					"Double01":   "CustomDoubleField",
+					"Int01":      "CustomIntField",
+					"Keyword01":  "CustomKeywordField",
+					"Text01":     "CustomTextField",
+				},
+			},
+		})
+		s.NoError(err)
+		// Wait for namespace cache to pick the UpdateNamespace changes.
+		time.Sleep(cacheRefreshInterval)
+	}
+


Why not move this to SetupSuite where it adds the search attributes to ES?

Apparently it tests namespace creation in the test. Probably it should be moved out. One day.

dnr

This all makes sense to me. Thanks for cleaning it up!

dnr · 2023-02-10T18:41:09Z

tests/advanced_visibility_test.go

@@ -1737,7 +1781,11 @@ func (s *elasticsearchIntegrationSuite) TestUpsertWorkflowExecution_InvalidKey()
 	_, err := poller.PollAndProcessWorkflowTask(false, false)
 	s.Error(err)
 	s.IsType(&serviceerror.InvalidArgument{}, err)
-	s.Equal("BadSearchAttributes: search attribute INVALIDKEY is not defined", err.Error())
+	if s.isElasticsearchEnabled() {
+		s.Equal("BadSearchAttributes: search attribute INVALIDKEY is not defined", err.Error())


Suggested change

s.Equal("BadSearchAttributes: search attribute INVALIDKEY is not defined", err.Error())

s.ErrorContains(err, "BadSearchAttributes: search attribute INVALIDKEY is not defined")

dnr · 2023-02-10T18:42:06Z

tests/advanced_visibility_test.go

+	if s.isElasticsearchEnabled() {
+		s.Equal("BadSearchAttributes: search attribute INVALIDKEY is not defined", err.Error())
+	} else {
+		s.Equal(fmt.Sprintf("BadSearchAttributes: Namespace %s has no mapping defined for search attribute INVALIDKEY", s.namespace), err.Error())


Suggested change

s.Equal(fmt.Sprintf("BadSearchAttributes: Namespace %s has no mapping defined for search attribute INVALIDKEY", s.namespace), err.Error())

s.ErrorContains(err, "has no mapping defined for search attribute INVALIDKEY")

I would leave namespace there.

dnr · 2023-02-10T18:45:16Z

tests/testdata/integration_test_cluster.yaml

@@ -4,5 +4,6 @@ historyconfig:
  numhistoryshards: 4
  numhistoryhosts: 1
 workerconfig:
+  startworkeranyway: true


I'm tempted to get rid of this setting and always start a worker, I mean, why not? but you don't have to do that

There are so many things that I'm tempted to get rid of :-). Agree, this needs to be done.

Yeah but I added this one and feel bad about it

dnr · 2023-02-10T18:48:02Z

tests/xdc/advanced_visibility_test.go

+	saListRequest = &workflowservice.ListWorkflowExecutionsRequest{
+		Namespace: namespace,
+		PageSize:  int32(2),
+		Query:     fmt.Sprintf(`WorkflowId = "%s" and %s = "another string"`, id, s.testSearchAttributeKey),


this is fine, but there are a few instances in non-test code now

alexshtin force-pushed the feature/adv-sql-tests branch 4 times, most recently from 15004aa to 270142f Compare February 10, 2023 00:27

dnr reviewed Feb 10, 2023

View reviewed changes

Enable Elasticsearch functional tests for SQL advanced visibility

9ff9885

alexshtin force-pushed the feature/adv-sql-tests branch from 270142f to 9ff9885 Compare February 10, 2023 06:24

alexshtin added 2 commits February 9, 2023 23:50

Cleanup cluster configs

e8042c6

Disable cyclomatic and cognitive linter rules for tests dir

1941091

alexshtin marked this pull request as ready for review February 10, 2023 08:15

alexshtin requested a review from a team as a code owner February 10, 2023 08:15

MichaelSnowden reviewed Feb 10, 2023

View reviewed changes

rodrigozhou reviewed Feb 10, 2023

View reviewed changes

rodrigozhou approved these changes Feb 10, 2023

View reviewed changes

dnr approved these changes Feb 10, 2023

View reviewed changes

yiminc merged commit 74bfaff into temporalio:master Feb 10, 2023

alexshtin deleted the feature/adv-sql-tests branch February 10, 2023 21:26

alexshtin mentioned this pull request Feb 10, 2023

Address #3923 feedback #3938

Merged

alexshtin added a commit to alexshtin/temporal that referenced this pull request Feb 10, 2023

Address temporalio#3923 feedback

e6f5e6f

alexshtin added a commit to alexshtin/temporal that referenced this pull request Feb 10, 2023

Address temporalio#3923 feedback

4e2fcb6

alexshtin added a commit to alexshtin/temporal that referenced this pull request Feb 11, 2023

Address temporalio#3923 feedback

24adae8

alexshtin added a commit that referenced this pull request Feb 11, 2023

Address #3923 feedback (#3938)

85c4381

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Elasticsearch functional tests for SQL advanced visibility #3923

Enable Elasticsearch functional tests for SQL advanced visibility #3923

alexshtin commented Feb 9, 2023 •

edited

Loading

dnr Feb 10, 2023

alexshtin Feb 10, 2023 •

edited

Loading

dnr Feb 10, 2023

alexshtin Feb 10, 2023

dnr Feb 10, 2023

rodrigozhou Feb 10, 2023

MichaelSnowden Feb 10, 2023

alexshtin Feb 10, 2023 •

edited

Loading

rodrigozhou Feb 10, 2023

alexshtin Feb 10, 2023

rodrigozhou Feb 10, 2023

alexshtin Feb 10, 2023

rodrigozhou Feb 10, 2023

alexshtin Feb 10, 2023

rodrigozhou Feb 10, 2023

alexshtin Feb 10, 2023

dnr left a comment

dnr Feb 10, 2023

dnr Feb 10, 2023

alexshtin Feb 10, 2023

dnr Feb 10, 2023

alexshtin Feb 10, 2023

dnr Feb 10, 2023

dnr Feb 10, 2023

	s.Equal("BadSearchAttributes: search attribute INVALIDKEY is not defined", err.Error())
	s.ErrorContains(err, "BadSearchAttributes: search attribute INVALIDKEY is not defined")

	s.Equal(fmt.Sprintf("BadSearchAttributes: Namespace %s has no mapping defined for search attribute INVALIDKEY", s.namespace), err.Error())
	s.ErrorContains(err, "has no mapping defined for search attribute INVALIDKEY")

Enable Elasticsearch functional tests for SQL advanced visibility #3923

Enable Elasticsearch functional tests for SQL advanced visibility #3923

Conversation

alexshtin commented Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

alexshtin Feb 10, 2023 • edited Loading

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

alexshtin Feb 10, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexshtin commented Feb 9, 2023 •

edited

Loading

alexshtin Feb 10, 2023 •

edited

Loading

alexshtin Feb 10, 2023 •

edited

Loading