Refactor for DRY state getters_validator_test and getters_test

[leolara]

What type of PR is this?

Refactor

What does this PR do? Why is it needed?

There are many code repetition on state unit tests, this is an example of a way of fixing this

Which issues(s) does this PR fix?

Partly #10047

Other notes for review

If this approach is seen as good, I will do the same with all the tests in state package while extending coverage

[terencechain]

Suggested change

	package testtmpl
	package testing

or test

[leolara]

@terencechain my idea is to create a package not general for testing, but for the specific goal of having all the test code that are repeated in many subpackages of state

[terencechain]

testing here refers to the test template too; it’s more idiomatic and follows the rest of the repo

[terencechain]

Suggested change

	type testTmplBeaconState_ValidatorAtIndexReadOnly_HandlesNilSlice_factory func() (state.BeaconState, error)
	type getState func() (state.BeaconState, error)

[leolara]

@terencechain getState would be global for the package, and it clash with other factories necessary for other tests templates

[terencechain]

It's local within the pkg, and it is better given the function signatures. We can make it more explicit if there really is a need (which I don't believe) later

[terencechain]

Suggested change

	func TestTmplBeaconState_ValidatorAtIndexReadOnly_HandlesNilSlice(
	func TestBeaconState_ValidatorAtIndexReadOnly_HandlesNilSlice(

[leolara]

@terencechain there would be no way of disguising a test from the test template by the function name

[terencechain]

Suggested change

	fact testTmplBeaconState_ValidatorAtIndexReadOnly_HandlesNilSlice_factory,
	st getState,

[leolara]

@terencechain

So my general idea is to show here an example for a specific pattern that will have to be repeated many times.

As there are v1, v2, v3, state-static/v[1..3] we will have to do this with most of the testing code.

My idea is:

• to have a specific package for these "test patterns" separated from other test helpers
• to follow a common pattern on how these are written:
  ++ Use the same name of the function but instead of starting with Test start with TestTmpl
  ++ For the factories use as base of the type the name of the test template function, this factory type can be private as only the callee TestTmpl needs to use it in its definition, but the caller can just pass the factory function.
  ++ Use the same file name but without _test at the end
  ++ This is to avoid clashes of names and make more mechanic the transition to the new DRY system.

This idea can be modified of course, but I would like to try to keep these principles of having a mechanical relationship between the names of the repeated tests and the test templates, and have it in its own package separated from other generic test helpers.

[terencechain]

@terencechain

So my general idea is to show here an example for a specific pattern that will have to be repeated many times.

As there are v1, v2, v3, state-static/v[1..3] we will have to do this with most of the testing code.

My idea is:

• to have a specific package for these "test patterns" separated from other test helpers
• to follow a common pattern on how these are written:
  ++ Use the same name of the function but instead of starting with Test start with TestTmpl
  ++ For the factories use as base of the type the name of the test template function, this factory type can be private as only the callee TestTmpl needs to use it in its definition, but the caller can just pass the factory function.
  ++ Use the same file name but without _test at the end
  ++ This is to avoid clashes of names and make more mechanic the transition to the new DRY system.

This idea can be modified of course, but I would like to try to keep these principles of having a mechanical relationship between the names of the repeated tests and the test templates, and have it in its own package separated from other generic test helpers.

Sorry. Naming is hard, but I'd love to be a little more idiomatic towards Go and around what Prysm has done so far.

• Most of our templates, setup helpers, test helpers are usually grouped within pkg testing under the target directory
• Instead of starting Test or TestTmpl. Maybe we can name it VerifyStateErrNilValidators. It's cleaner. Keep in mind that we will label this "test-only" via bazel so there's no worry about other code that references this. There won't be any crashing concern here

[terencechain]

Could you also please run gazelle and create the BUILD.bazel file?
1.) bazel run //:gazelle
2.) add testonly = True
3.) edit visibility to beacon chain subpkg or even beacon state state subpkg?

[leolara]

@terencechain so if we are already doing something similar? Could you send me a few links to the code so I see how it is done in other places and I do the same?

[terencechain]

@terencechain so if we are already doing something similar? Could you send me a few links to the code so I see how it is done in other places and I do the same?

Sure. There should be helpers in the below links that do the same

.

[leolara]

@terencechain please, check again

[leolara]

@terencechain @prestonvanloon @rkapka @nisdas please have a look, I also added BUILD.bazel that I forgot previously

[leolara]

Could you also please run gazelle and create the BUILD.bazel file? 1.) bazel run //:gazelle 2.) add testonly = True 3.) edit visibility to beacon chain subpkg or even beacon state state subpkg?

This is done

[nisdas]

You do not need to separate out for our native state. They all follow the same interface, you can use the above test for the native state. This applies to all the below following tests.

[leolara]

@nidas The Native one does not have clear method, that is why I indroduced this

[nisdas]

You can just have the native state clear method to do nothing so as to reduce test duplication here.

[leolara]

If I am not understanding something from your comment, let's fix it in another PR, so this does not get in more conflicts

[nisdas]

Maybe it will be clearer with an example:

func TestBeaconState_MatchCurrentJustifiedCheckpt(t *testing.T) {
	testtmpl.VerifyBeaconState_MatchCurrentJustifiedCheckpt(
		t,
		func(cp *ethpb.Checkpoint) (state.BeaconState, error) {
			return InitializeFromProto(&ethpb.BeaconState{CurrentJustifiedCheckpoint: cp})
		},
func(i state.BeaconState) { 
}
	)
}

Basically the clear function is no-op .

[leolara]

@rkapka it seems it is waiting for your review

[rkapka]

@rkapka it seems it is waiting for your review

Either mine or Nishant's. Please resolve his comment and I'm sure he will approve.

[nisdas]

Approving it, as you cant abstract out the native state tests without it failing. We can look at this again later

[rkapka]

@leolara We need bazel run //:gazelle -- fix

[leolara]

@leolara We need bazel run //:gazelle -- fix

Ok

[leolara]

@leolara We need bazel run //:gazelle -- fix

Done

[leolara]

@rkapka waiting for your approval