[BCI-3989][common] - CR methods err when service unstarted by Farber98 · Pull Request #705 · smartcontractkit/chainlink-common

Farber98 · 2024-08-14T16:48:16Z

solana ref: BCI-3989-cr-methods-error-when-unstarted
core ref: BCI-3989-cr-methods-error-when-unstarted

Task Description:

We only want GetLatestValue, BatchGetLatestValue and QueryKey to be called when CR service is in Started state. If CR is not started yet, we should return an error.

This PR:

Ticket:

BCI-3989

Unblocks:

Merging flow:

merge common (point to solana and core for CI to pass)
Then merge Solana by pointing to merged common
Finally merge core after changing Solana and common refs

This reverts commit 5006bec.

…eded

ilija42 · 2024-09-04T17:01:30Z

pkg/loop/internal/relayer/pluginprovider/chainreader/chain_reader_test.go

+func (it *fakeChainReaderInterfaceTester) Start(t *testing.T) {
+	fake, ok := it.impl.(*fakeChainReader)
+	assert.True(t, ok)
+	require.NoError(t, fake.Start(context.Background()))
+}
+
+func (it *fakeChainReaderInterfaceTester) Close(t *testing.T) {
+	fake, ok := it.impl.(*fakeChainReader)
+	assert.True(t, ok)
+	require.NoError(t, fake.Close())
+}


Why do we need these, why not directly call Start and Close on the ChainReader returned from GetChainReader

It fails when we run the LOOPs tests. It tries to execute this Start() func which is a no-op. Exposing these 2 funcs in the InterfaceTester was a workaround on this issue.

Let me know your thoughts if there's other way of achieving this without exposing these methods. I'm not very familiar with LOOPs yet and might be missing some context

Casting the interface to a specific struct type does not affect which method is called by name via Start/Close. Please see my other comment: #705 (comment)

jmank88 · 2024-09-13T11:58:50Z

pkg/loop/internal/relayer/pluginprovider/contractreader/contract_reader_test.go

+func (it *fakeContractReaderInterfaceTester) StartContractReader(t *testing.T) {
+	fake, ok := it.impl.(*fakeContractReader)
+	assert.True(t, ok)
+	require.NoError(t, fake.Start(context.Background()))
+}
+
+func (it *fakeContractReaderInterfaceTester) CloseContractReader(t *testing.T) {
+	fake, ok := it.impl.(*fakeContractReader)
+	assert.True(t, ok)
+	require.NoError(t, fake.Close())
+}


I'm having trouble seeing why we need these helper methods. impl is already a ContractReader, so we don't need to cast it to a specific fake type:

Suggested change

func (it *fakeContractReaderInterfaceTester) StartContractReader(t *testing.T) {

fake, ok := it.impl.(*fakeContractReader)

assert.True(t, ok)

require.NoError(t, fake.Start(context.Background()))

}

func (it *fakeContractReaderInterfaceTester) CloseContractReader(t *testing.T) {

fake, ok := it.impl.(*fakeContractReader)

assert.True(t, ok)

require.NoError(t, fake.Close())

}

func (it *fakeContractReaderInterfaceTester) StartContractReader(t *testing.T) {

require.NoError(t, it.impl.Start(context.Background()))

}

func (it *fakeContractReaderInterfaceTester) CloseContractReader(t *testing.T) {

require.NoError(t, it.impl.Close())

}

Additionally, there is already a GetContractReader method, so callers that are doing this:

tester.StartContractReader(t) defer tester.CloseContractReader(t)

Could instead do this:

tester.GetContractReader(t).Start(ctx) defer tester.GetContractReader(t).Close()

Or better yet:

servicetest.Run(t, tester.GetContractReader(t))

@jmank88 @silaslenihan I couldn't do it this way. Tests fail for the loop relay client.

Pushed some changes with a new idea:

Modify testCases and Setup to receive a startCR flag and control whether we should start CR or not

With this, Start and Close will be handled by Setup with Cleanup

All tests are passing, including new ones where we test that the service does not start. Let me know your thoughts

Could you please update the description? I'm having trouble understanding the fundamental problem we are trying to solve.

Since the BasicTester interface is used more generally than for just contract reader, I don't think it makes sense to add a contract-reader-only argument to BasicTester.Setup.

Fundamental problem we are trying to solve: We only want GetLatestValue, BatchGetLatestValue and QueryKey to be called when the service CR is in Started state. If CR has not yet been started, we should return an error.

To test this new behavior, we need a way to get the CR in both states (started and not started).

We can't just call Start and Close directly on the return of GetContractReader to achieve this. This is because we also run relay client loop tests here. We are calling Start and Close on the client and not on the server as would be required.

The two solutions I proposed were aimed at solving this problem, the one with the helper methods and now the one with the flag. I feel like the one with the flag is cleaner. We are providing in the testCase the startCR flag to determine if the underlying Setup function should return the service in started or unstarted state

What do you think about defining a new interface similar to BasicTester but use it specifically on contract-reader?

We can't just call Start and Close directly on the return of GetContractReader to achieve this. This is because we also run relay client loop tests here. We are calling Start and Close on the client and not on the server as would be required.

The two solutions I proposed were aimed at solving this problem, the one with the helper methods and now the one with the flag.

Something isn't adding up here. The modification that I proposed to your helper methods was intended to execute exactly equivalently. It was not proposing a change in behavior - definitely not one that switched from client to server or vice versa. If the helper method solution was valid, then we should revisit it, but in the most simplified form.

Pushed a new solution with interfaces refactor and comments on changes. If do you think it's okay we can go with it. Else, I'll revisit the one with the helper methods in the most simplified form

jmank88

It looks like there are still some simplifications available

jmank88 · 2024-09-16T13:33:59Z

pkg/loop/internal/relayer/pluginprovider/contractreader/contract_reader_test.go

+			require.NoError(t, it.impl.Start(context.Background()))
+			t.Cleanup(func() { require.NoError(t, it.impl.Close()) })


Please use servicetest.Run. Notice it uses assert during cleanup.

…rted" This reverts commit 54d7016, reversing changes made to 4e67d8d.

…arted" This reverts commit e728084.

Farber98 · 2024-09-17T11:00:37Z

pkg/types/interfacetests/chain_components_interface_tests.go

 )

-type ChainComponentsInterfaceTester[T TestingT[T]] interface {
+type ChainComponentsTester[T any] interface {


defined ChainComponentsTester so we use the Setup(t T, startCR bool) only for chainComponents tests

defined chainComponentsTestcase so we use the startCR testcase flag only for chainComponents testcases

defined runChainComponentsTests to execute chainComponents specific tests using the ChainComponentsTester interface with Setup(t T, startCR bool)

Farber98 · 2024-09-17T11:09:10Z

pkg/types/interfacetests/codec_interface_tests.go

 	TestOn       string
 }

+type codecTestcase[T any] struct {


defined CodecTester so we use the Setup(t T) without the startCR flag for the codec

defined codecTestcase so we don't use the startCR testcase flag for codec testcases

defined runCodecTests to execute codec specific tests using the CodecTester interface with Setup(t T)

Farber98 · 2024-09-17T11:14:06Z

pkg/loop/internal/relayer/pluginprovider/contractreader/contract_reader_test.go

 		fake.stored = []TestStruct{}
+
+		if startCR {
+			servicetest.Run(t, it.impl)


added servicetest.Run(t, it.impl)

ilija42

Wdyt about this solution

#777
smartcontractkit/chainlink#14480

check cr service is started

352adfe

Farber98 temporarily deployed to integration August 14, 2024 16:48 — with GitHub Actions Inactive

Farber98 added 2 commits August 15, 2024 10:19

small refactor

173eca4

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

e16b694

Farber98 temporarily deployed to integration August 15, 2024 13:20 — with GitHub Actions Inactive

Farber98 had a problem deploying to integration August 15, 2024 13:20 — with GitHub Actions Error

Farber98 temporarily deployed to integration August 15, 2024 13:20 — with GitHub Actions Inactive

remove unused error

0d8a1b0

Farber98 temporarily deployed to integration August 15, 2024 13:24 — with GitHub Actions Inactive

bind before start

af1fabf

Farber98 temporarily deployed to integration August 15, 2024 14:23 — with GitHub Actions Inactive

Farber98 added 2 commits August 15, 2024 12:42

add comments to tests

4dcb052

startChainReader method

4d966e6

Farber98 temporarily deployed to integration August 15, 2024 15:45 — with GitHub Actions Inactive

Farber98 had a problem deploying to integration August 15, 2024 15:45 — with GitHub Actions Failure

Farber98 temporarily deployed to integration August 15, 2024 15:45 — with GitHub Actions Inactive

check methods return error when called without starting service

5006bec

Farber98 had a problem deploying to integration August 16, 2024 02:36 — with GitHub Actions Failure

Farber98 temporarily deployed to integration August 16, 2024 02:36 — with GitHub Actions Inactive

Revert "check methods return error when called without starting service"

cc3066b

This reverts commit 5006bec.

Farber98 had a problem deploying to integration August 16, 2024 03:03 — with GitHub Actions Failure

Farber98 temporarily deployed to integration August 16, 2024 03:03 — with GitHub Actions Inactive

Farber98 temporarily deployed to integration August 28, 2024 16:32 — with GitHub Actions Inactive

Farber98 had a problem deploying to integration August 28, 2024 16:32 — with GitHub Actions Failure

Farber98 had a problem deploying to integration August 29, 2024 14:44 — with GitHub Actions Failure

Farber98 had a problem deploying to integration August 29, 2024 15:00 — with GitHub Actions Failure

Farber98 had a problem deploying to integration August 29, 2024 15:00 — with GitHub Actions Error

Farber98 had a problem deploying to integration August 29, 2024 15:04 — with GitHub Actions Failure

Farber98 added 5 commits August 29, 2024 16:21

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

50c55ef

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

699bdf8

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

fa1ace8

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

bed6fbf

add start method to the interface. add start and close calls where ne…

208ef02

…eded

ilija42 reviewed Sep 4, 2024

View reviewed changes

Farber98 added 4 commits September 5, 2024 09:41

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

a453d2d

fix conflicts with chain components pr

0d7af20

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

647bdde

fix pr conflicts

489fd0a

jmank88 reviewed Sep 13, 2024

View reviewed changes

jmank88 requested changes Sep 13, 2024

View reviewed changes

Farber98 added 3 commits September 15, 2024 20:31

add flag to control if we return cr started

e8ed82b

fix encodings iface

66ba7cd

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

392195f

jmank88 reviewed Sep 16, 2024

View reviewed changes

Farber98 added 4 commits September 16, 2024 18:12

refactor codec and chaincomponents ifaces

4e67d8d

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

54d7016

Revert "Merge branch 'main' into BCI-3989-cr-methods-error-when-unsta…

e728084

…rted" This reverts commit 54d7016, reversing changes made to 4e67d8d.

Reapply "Merge branch 'main' into BCI-3989-cr-methods-error-when-unst…

953d786

…arted" This reverts commit e728084.

Farber98 commented Sep 17, 2024

View reviewed changes

Merge branch 'main' into BCI-3989-cr-methods-error-when-unstarted

f94c2c9

ilija42 requested changes Sep 18, 2024

View reviewed changes

		require.NoError(t, it.impl.Start(context.Background()))
		t.Cleanup(func() { require.NoError(t, it.impl.Close()) })

Conversation

Farber98 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Task Description:

This PR:

Ticket:

Unblocks:

Merging flow:

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmank88 Sep 16, 2024 • 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!

Farber98 Sep 16, 2024 • 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!

Farber98 Sep 17, 2024 • 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!

jmank88 left a comment

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!

ilija42 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Farber98 commented Aug 14, 2024 •

edited

Loading

jmank88 Sep 16, 2024 •

edited

Loading

Farber98 Sep 16, 2024 •

edited

Loading

Farber98 Sep 17, 2024 •

edited

Loading