add failpoint raftBeforeAdvance to reproduce TestMemberPromoteMemberNotLearner reliably

[chaochn47]

REF: #15708 (comment) and #15708 (comment)

Since gofailpoint isn’t enabled by default for the existing e2e test cases, so we will create a separate test suite under a new package “go.etcd.io/etcd/tests/v3/negative”(preferred) or “go.etcd.io/etcd/tests/v3/failpoint”

REF: Terminate etcd on stalled storage writes Google doc Test section

To reproduce

make gofail-enable 
pushd tests/failpoint 
go test -v -run TestMemberPromoteMemberNotLearnerWithFailpoint
popd

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

[ahrtr]

It doesn't make much sense to define an interface, because failpointFunc is only used in the same package. It makes sense if you move the interface into the test framework.

We can keep it as it's for now, and discuss it separately.

[chaochn47]

Update - Actions customers may see an increase in delayed runs, and some customers may see issues creating PRs and starting or resuming Codespaces. We continue to investigate and drive towards a mitigation.
May 10, 2023 - 16:17 UTC

https://www.githubstatus.com/

[ahrtr]

Overall looks good to me.

Thanks @chaochn47

To be clearer, currently only the robustness test cases are executed with failpoint enabled by default. All test cases under tests/failpoint will also be executed with failpoint enabled by default, and a followup PR is required to update the Makefile and workflow to enable gofailpoint for cases under tests/failpoint.

[serathius]

I don't see the reason for creating another test type. For me this is an integration test that should skipped ('t.Skip`) if we didn't compile with gofail. For CI integration we should just enable gofail before running all tests.

[chaochn47]

I don't see the reason for creating another test type. For me this is an integration test that should skipped ('t.Skip`) if we didn't compile with gofail. For CI integration we should just enable gofail before running all tests.

Thanks @serathius! The conversation is moved to #15879. Hopefully there is an agreement on how to approach this.

[chaochn47]

Hi @serathius, seems like from #15879 majority of commenters agree with adding the limited number of failpoint type of tests just like https://github.com/etcd-io/bbolt/tree/master/tests/failpoint. Do you have strong objections of this direction?

Otherwise, can we let the PR merge and continue fixing #15528, #14040, #13212, #12983?

[serathius]

Don't agree that we need a separate failpoint test type, nor a separate test directory, not a separate testing target.
My recommendation. Make the test check if the failpoint it depends on is available, if not then skip and enable gofailpoint in the workflow tests.

[chaochn47]

okay. That should also work given just compiling etcd integration test with go fail should not produce any new issues. I can start from that and circle back on this topic if necessary.