Refactor: use namespace.Name and namespace.ID in tests #7065

alexshtin · 2025-01-10T07:08:18Z

What changed?

Refactor: use namespace.Name and namespace.ID instead of string in tests.

Why?

Special narrow types should be used where possible.

How did you test it?

Run tests.

Potential risks

No risks.

Documentation

No.

Is hotfix candidate?

No.

alexshtin · 2025-01-10T07:35:53Z

tests/testcore/functional_test_base.go

+		namespace              namespace.Name
+		namespaceID            namespace.ID
+		foreignNamespace       namespace.Name
+		archivalNamespace      namespace.Name
+		archivalNamespaceID    namespace.ID


This is a change.

namespaceName.
foreignNamespaceName.
etc.

otherwise - why namespaceID for namespace.ID but just namespace for namespace.Name?

Name or ID can be inferred from type of the field (one of the reason of this PR)

We all get used to just "namespace" mostly means name.

lets be clear and consistent, and think about future readers.
We have Namespace struct, so no, it is not clear that 'namespace` is name.
so if this represents namespace.Name - please rename this to namespaceName.

ychebotarev · 2025-01-10T18:10:00Z

tests/archival_test.go

@@ -321,7 +316,8 @@ func (s *ArchivalSuite) isMutableStateDeleted(namespaceID string, execution *com
 }

 func (s *ArchivalSuite) startAndFinishWorkflow(
-	id, wt, tq, namespace, namespaceID string,
+	id, wt, tq string,
+	namespace namespace.Name,


same here. You are sending in namespace.Name structure, but using only namespace name as a string.
Just send string (as it was before).
You can rename it to namespaceName or nsName

ychebotarev · 2025-01-10T18:19:48Z

tests/testcore/functional_test_base.go

+		namespace              namespace.Name
+		namespaceID            namespace.ID
+		foreignNamespace       namespace.Name
+		archivalNamespace      namespace.Name
+		archivalNamespaceID    namespace.ID


namespaceName.
foreignNamespaceName.
etc.

otherwise - why namespaceID for namespace.ID but just namespace for namespace.Name?

ychebotarev · 2025-01-10T18:20:01Z

tests/testcore/functional_test_base.go

@@ -127,15 +130,23 @@ func (s *FunctionalTestBase) HttpAPIAddress() string {
 	return s.httpAPIAddress
 }

-func (s *FunctionalTestBase) Namespace() string {
+func (s *FunctionalTestBase) Namespace() namespace.Name {


NamespaceName()

also return string. There is no point of returning napespace.name structure, given that it is a thin wrapper over stirng.

There is a point. And this is a goal of this PR. Both Namespace.ID and Namespace.Name are string, and it is easy to mess the up. Having distinct types in the code helps this. Major part of server code is using those distinct types already. This is just a follow-up for tests.

I don't see what is easy to mess up in
func (s *FunctionalTestBase) NamespaceName() string {
?

or in NamespaceID() string.

we are mostly working with strings, but you can't use namespace.Name directly, you still need to call String().
So no, namespace.Name is not a string.

s.Namespace().String() is unnecessary verbose, and not adding anything.
s.NamespaceName() is much cleaner.

ychebotarev · 2025-01-10T18:20:21Z

tests/testcore/functional_test_base.go

+	return s.namespaceID
+}
+
+func (s *FunctionalTestBase) ArchivalNamespace() namespace.Name {


ArchivalNamespaceName()

ychebotarev · 2025-01-10T18:20:34Z

tests/testcore/functional_test_base.go

+	return s.archivalNamespaceID
+}
+
+func (s *FunctionalTestBase) ForeignNamespace() namespace.Name {


ForeignNamespaceName

ychebotarev · 2025-01-10T18:27:30Z

tests/testcore/functional_test_base.go

-	s.namespace = RandomizeStr("namespace")
-	s.Require().NoError(s.registerNamespaceWithDefaults(s.namespace))
+	s.namespace = namespace.Name(RandomizeStr("namespace"))
+	s.namespaceID, err = s.registerNamespaceWithDefaults(s.Namespace())


this is the only place where you actually assign namespace.ID to something.
You may just add the code that gets namespace ID from namespace here, and avoid returning something that is mostly not used.

ychebotarev · 2025-01-10T18:28:00Z

tests/testcore/functional_test_base.go

@@ -355,17 +369,28 @@ func (s *FunctionalTestBase) registerNamespace(
 			},
 		},
 	})
+	if err != nil {


you are not using this value in anywhere directly
remove this return value.

There are asserts that err is nil on caller side.

ychebotarev · 2025-01-10T18:28:22Z

tests/testcore/functional_test_base.go


-	return err
+	descResp, err := s.client.DescribeNamespace(ctx, &workflowservice.DescribeNamespaceRequest{


add this code to that only place where you actually need namespace ID

ychebotarev · 2025-01-10T21:06:16Z

tests/activity_api_pause_test.go

@@ -134,7 +133,7 @@ func (s *ActivityApiPauseClientTestSuite) TestActivityPauseApi_WhileRunning() {

 	// pause activity
 	pauseRequest := &workflowservice.PauseActivityByIdRequest{
-		Namespace:  s.Namespace(),
+		Namespace:  s.Namespace().String(),


this is more inconvenient then before.

More typing, yes. More inconvenient, no.

ychebotarev · 2025-01-10T21:10:10Z

tests/testcore/functional_test_base.go

 }

 func (s *FunctionalTestBase) markNamespaceAsDeleted(
-	namespace string,
+	nsName namespace.Name,


here and everywhere below.
What is the benefit of changing string to namespace.Name?
You are only using it as nsName.String()

I replied in another comment.

stephanos · 2025-01-10T21:12:34Z

tests/testcore/functional_test_base.go

+	descResp, err := s.client.DescribeNamespace(ctx, &workflowservice.DescribeNamespaceRequest{
+		Namespace: nsName.String(),
+	})
+


Suggested change

It is not there anymore.

alexshtin requested a review from a team as a code owner January 10, 2025 07:08

alexshtin commented Jan 10, 2025

View reviewed changes

alexshtin added 3 commits January 10, 2025 12:59

Refactor: use namespace.Name and namespace.ID in tests

e6973ce

Fix tests

57db724

Fix linter

42f3e98

alexshtin force-pushed the refactor/namespace-tests branch from 4948d49 to 42f3e98 Compare January 10, 2025 20:59

ychebotarev reviewed Jan 10, 2025

View reviewed changes

stephanos approved these changes Jan 10, 2025

View reviewed changes

Unify registerNamespace function

c7810a2

alexshtin merged commit 720505d into temporalio:main Jan 11, 2025
49 checks passed

alexshtin deleted the refactor/namespace-tests branch January 11, 2025 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: use namespace.Name and namespace.ID in tests #7065

Refactor: use namespace.Name and namespace.ID in tests #7065

alexshtin commented Jan 10, 2025 •

edited

Loading

alexshtin Jan 10, 2025

ychebotarev Jan 10, 2025

alexshtin Jan 11, 2025

ychebotarev Jan 11, 2025

ychebotarev Jan 10, 2025 •

edited

Loading

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

alexshtin Jan 11, 2025

ychebotarev Jan 11, 2025

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

alexshtin Jan 11, 2025

ychebotarev Jan 10, 2025

ychebotarev Jan 10, 2025

alexshtin Jan 11, 2025

ychebotarev Jan 10, 2025

alexshtin Jan 11, 2025

stephanos Jan 10, 2025

alexshtin Jan 11, 2025


		return err
		descResp, err := s.client.DescribeNamespace(ctx, &workflowservice.DescribeNamespaceRequest{

Refactor: use namespace.Name and namespace.ID in tests #7065

Refactor: use namespace.Name and namespace.ID in tests #7065

Conversation

alexshtin commented Jan 10, 2025 • edited Loading

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

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

ychebotarev Jan 10, 2025 • 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

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

alexshtin commented Jan 10, 2025 •

edited

Loading

ychebotarev Jan 10, 2025 •

edited

Loading