Mutation testing #28

JekaMas · 2022-10-06T08:59:22Z

Description

As far as consensus is extremely sensitive functionality, I believe we need a way to check our tests for usefulness and correctness. Code coverage, unfortunately, doesn't tell us about the quality of tests, either about usefulness. Even with high code coverage it's possible to write not very useful tests that always 'green', or tests that are 'green' if being executed in particular order.
For that reason the PR introduces changes:

As far as we run tests with t.Parallel() it makes sense to use -race flag as well. At the moment tests do have data races that'd be great to fix in separate issues.
To prevent incorrect global state the PR introduces -shuffle flag
To check that tests are useful the PR adds mutation testing https://github.com/avito-tech/go-mutesting

Changes include

Bugfix (non-breaking change that solves an issue)
Hotfix (change that solves an urgent issue, and requires immediate attention)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

I have assigned this PR to myself
I have added at least 1 reviewer
I have added the relevant labels
I have added sufficient documentation in code

Comments

This PR is for discussion about new tooling. At the moment data race tests and mutation tests are red. If we'd like to merge the PR, we should comment out this CI steps and make tasks to fix tests with race and a bunch of tasks to add/check tests with respect of mutation testing results.

A few examples of mutation testing:

 func (i *IBFT) handlePrepare(view *proto.View, quorum uint64) bool {
 	isValidPrepare := func(message *proto.Message) bool {
 		// Verify that the proposal hash is valid
@@ -799,7 +799,7 @@
 
 	if len(prepareMessages) < int(quorum)-1 {
 		//	quorum not reached, keep polling
-		return false
+
 	}
 
func (i *IBFT) handlePrepare(view *proto.View, quorum uint64) bool {
 	isValidPrepare := func(message *proto.Message) bool {
 		// Verify that the proposal hash is valid
@@ -852,7 +852,7 @@
 		case <-sub.SubCh:
 			if !i.handleCommit(view, quorum) {
 				//	quorum not reached, retry
-				continue
+
 			}
 

 func (i *IBFT) handleCommit(view *proto.View, quorum uint64) bool {
 	isValidCommit := func(message *proto.Message) bool {
 		var (
@@ -880,7 +880,7 @@
 	commitMessage
[result.txt](https://github.com/0xPolygon/go-ibft/files/9723038/result.txt)
s := i.messages.GetValidMessages(view, proto.MessageType_COMMIT, isValidCommit)
 	if len(commitMessages) < int(quorum) {
 		//	quorum not reached, keep polling
-		return false
+
 	}
 


 func (i *IBFT) runStates(ctx context.Context) {
 	var timeout error
 
@@ -594,7 +594,7 @@
 
 	//	is proposer
 	if !i.backend.IsProposer(msg.From, height, round) {
-		return false
+
 	}

 func (i *IBFT) runStates(ctx context.Context) {
 	var timeout error
 
@@ -615,7 +615,7 @@
 
 	//	proposal must be for round 0
 	if msg.View.Round != 0 {
-		return false
+
 	}
 
 func (i *IBFT) handlePrePrepare(view *proto.View) *proto.Message {
 	isValidPrePrepare := func(message *proto.Message) bool {
 		if view.Round == 0 {
@@ -772,7 +772,7 @@
 		case <-sub.SubCh:
 			if !i.handlePrepare(view, quorum) {
 				//	quorum of valid prepare messages not received, retry
-				continue
+
 			}

More results are in the file result.txt

codecov-commenter · 2022-10-06T09:18:14Z

Codecov Report

Merging #28 (da087ce) into main (3e9fecd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #28   +/-   ##
=======================================
  Coverage   94.06%   94.06%           
=======================================
  Files           6        6           
  Lines        1314     1314           
=======================================
  Hits         1236     1236           
  Misses         57       57           
  Partials       21       21

Impacted Files	Coverage Δ
messages/event_manager.go	`100.00% <ø> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

vcastellm

✅
✅
Let's discuss

.github/workflows/main.yml

zivkovicmilos · 2022-10-07T10:57:42Z

@JekaMas
I'll review this PR along with @dbrajovic when it's ready, but just one note since you're still working on it:
Please don't drop the consensus_test.go file, and move those tests to ibft_test.go 🙏

There is a specific reason why we kept the 2 separate, as ibft_test.go contains only unit tests, whereas consensus_test.go contains cluster tests

JekaMas · 2022-10-11T14:06:57Z

@JekaMas I'll review this PR along with @dbrajovic when it's ready, but just one note since you're still working on it: Please don't drop the consensus_test.go file, and move those tests to ibft_test.go 🙏

There is a specific reason why we kept the 2 separate, as ibft_test.go contains only unit tests, whereas consensus_test.go contains cluster tests

I with I could keep this files separated. I left a comment, why this is needed. Otherwise, mutations won't be performed versus consensus_test

zivkovicmilos · 2022-10-13T08:24:35Z

Hey @JekaMas,

Resolved the races in tests in the following PR which I'll merge out to main:
PR #29

I've included your test race workflow there, so when you pull in the changes after it gets merged, you'll have them running

vcastellm · 2022-10-13T08:51:27Z

.github/workflows/main.yml

+      - name: Mutating testing
+        run: make mut
+
+#  test-race:


Why not use the -race flag in the existing workflow? https://github.com/0xPolygon/go-ibft/pull/28/files#diff-7829468e86c1cc5d5133195b5cb48e1ff6c75e3e9203777f6b2e379d9e4882b3R65

zivkovicmilos · 2022-10-13T08:52:58Z

Makefile

 lint:
 	golangci-lint run -E whitespace -E wsl -E wastedassign -E unconvert -E tparallel -E thelper -E stylecheck -E prealloc \
 	-E predeclared -E nlreturn -E misspell -E makezero -E lll -E importas -E ifshort -E gosec -E  gofmt -E goconst \
 	-E forcetypeassert -E dogsled -E dupl -E errname -E errorlint -E nolintlint --timeout 2m

+install:


What do you think about renaming this to install-mut, so it's more clear?

vcastellm · 2022-10-13T08:54:02Z

Makefile

+
+mut:
+	go-mutesting --blacklist=".github/mut_blacklist" --config=".github/mut_config.yml" ./...
+	@echo MSI: `jq '.stats.msi' report.json`


Nitpick: Missing new line

zivkovicmilos · 2022-10-13T09:08:12Z

@JekaMas
The comment just states that the files need to be joined up, and I can't find anything on the official docs about it.
Can you please explain the reasoning because I'm probably missing something?

JekaMas · 2022-10-13T10:11:12Z

@JekaMas The comment just states that the files need to be joined up, and I can't find anything on the official docs about it. Can you please explain the reasoning because I'm probably missing something?

Sure, it is from the mutation testing package readme https://github.com/avito-tech/go-mutesting

The targets of the mutation testing can be defined as arguments to the binary. Every target can be either a Go source file, a directory or a package. Directories and packages can also include the ... wildcard pattern which will search recursively for Go source files. Test source files with the suffix _test are excluded, since this would interfere with the testing process most of the time.

Also, I just tried it.

zivkovicmilos · 2022-10-14T06:13:45Z

@JekaMas

Sure, it is from the mutation testing package readme https://github.com/avito-tech/go-mutesting
The targets of the mutation testing can be defined as arguments to the binary. Every target can be either a Go source file, a directory or a package. Directories and packages can also include the ... wildcard pattern which will search recursively for Go source files. Test source files with the suffix _test are excluded, since this would interfere with the testing process most of the time.
Also, I just tried it.

Isn't it possible to just give the directory / package for mutation, instead of a single test file? I've ran it with ./...

JekaMas · 2022-10-14T08:04:22Z

@JekaMas

Sure, it is from the mutation testing package readme https://github.com/avito-tech/go-mutesting
The targets of the mutation testing can be defined as arguments to the binary. Every target can be either a Go source file, a directory or a package. Directories and packages can also include the ... wildcard pattern which will search recursively for Go source files. Test source files with the suffix _test are excluded, since this would interfere with the testing process most of the time.
Also, I just tried it.

Isn't it possible to just give the directory / package for mutation, instead of a single test file? I've ran it with ./...

It is possible, although it'd run file by file:

greb a file file.go
perform a mutation
run tests file_test.go
mark the result

JekaMas · 2022-10-14T10:17:33Z

@zivkovicmilos Could you check the last changes? I tried a different approach with test files.

zivkovicmilos · 2022-10-15T10:03:24Z

@JekaMas

@zivkovicmilos Could you check the last changes? I tried a different approach with test files.

I like this idea of having a test that will be the cornerstone for the cluster mutation tests 💯
It's much cleaner now

JekaMas · 2022-11-04T13:01:54Z

closed in favor of #38

JekaMas added 2 commits October 6, 2022 12:08

mutating and shuffle

1bbfa56

step name

3d6d6ba

JekaMas added the feature New functionality label Oct 6, 2022

JekaMas requested review from vcastellm, b00ris, zivkovicmilos, ferranbt, ZeljkoBenovic and dbrajovic October 6, 2022 08:59

JekaMas self-assigned this Oct 6, 2022

JekaMas marked this pull request as draft October 6, 2022 08:59

JekaMas added 2 commits October 6, 2022 13:06

tests with race

0126fd5

add install command to CI

eea1285

install

6d02edc

JekaMas mentioned this pull request Oct 6, 2022

Mutation testing maticnetwork/bor#543

Closed

vcastellm reviewed Oct 6, 2022

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

JekaMas added 2 commits October 11, 2022 18:01

comment out tests with data race detector

fcb2300

main merge

5ff0255

fix install

c439fbd

JekaMas marked this pull request as ready for review October 11, 2022 14:14

JekaMas requested a review from vcastellm October 12, 2022 05:54

zivkovicmilos mentioned this pull request Oct 13, 2022

Resolve data races in tests utilizing go routines #29

Merged

8 tasks

vcastellm reviewed Oct 13, 2022

View reviewed changes

zivkovicmilos reviewed Oct 13, 2022

View reviewed changes

vcastellm reviewed Oct 13, 2022

View reviewed changes

JekaMas added 3 commits October 14, 2022 12:44

filter out few false positive

b6d4885

main merge

a46e296

restore consensus_test.go

eb518fc

JekaMas added 14 commits October 15, 2022 17:24

makefile. add dependencies

67a53ce

makefile. add dependencies

0189b99

min msi

f8352cc

update mutations

b8aabf8

restore mut

6ff6ba2

fix regex vuln

cf776a1

fix regex vuln

c8f5baf

fix regex vuln

3907e7f

bump golang

b9c2709

makefile

e6d13db

bump golangci

ed43965

bump mutations

d8f9645

tidy

26a0f2b

tidy

19aec58

JekaMas requested review from vcastellm and zivkovicmilos November 3, 2022 09:11

JekaMas added 4 commits November 4, 2022 15:51

linters

d351981

merge

aa8b668

update mutations

824c2d2

comment

da087ce

JekaMas closed this Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutation testing #28

Mutation testing #28

JekaMas commented Oct 6, 2022 •

edited

Loading

codecov-commenter commented Oct 6, 2022 •

edited by codecov bot

Loading

vcastellm left a comment

zivkovicmilos commented Oct 7, 2022

JekaMas commented Oct 11, 2022

zivkovicmilos commented Oct 13, 2022

vcastellm Oct 13, 2022

zivkovicmilos Oct 13, 2022

vcastellm Oct 13, 2022

zivkovicmilos commented Oct 13, 2022

JekaMas commented Oct 13, 2022

zivkovicmilos commented Oct 14, 2022 •

edited

Loading

JekaMas commented Oct 14, 2022

JekaMas commented Oct 14, 2022 •

edited

Loading

zivkovicmilos commented Oct 15, 2022

JekaMas commented Nov 4, 2022

Mutation testing #28

Mutation testing #28

Conversation

JekaMas commented Oct 6, 2022 • edited Loading

Description

Changes include

Checklist

Comments

codecov-commenter commented Oct 6, 2022 • edited by codecov bot Loading

Codecov Report

vcastellm left a comment

Choose a reason for hiding this comment

zivkovicmilos commented Oct 7, 2022

JekaMas commented Oct 11, 2022

zivkovicmilos commented Oct 13, 2022

vcastellm Oct 13, 2022

Choose a reason for hiding this comment

zivkovicmilos Oct 13, 2022

Choose a reason for hiding this comment

vcastellm Oct 13, 2022

Choose a reason for hiding this comment

zivkovicmilos commented Oct 13, 2022

JekaMas commented Oct 13, 2022

zivkovicmilos commented Oct 14, 2022 • edited Loading

JekaMas commented Oct 14, 2022

JekaMas commented Oct 14, 2022 • edited Loading

zivkovicmilos commented Oct 15, 2022

JekaMas commented Nov 4, 2022

JekaMas commented Oct 6, 2022 •

edited

Loading

codecov-commenter commented Oct 6, 2022 •

edited by codecov bot

Loading

zivkovicmilos commented Oct 14, 2022 •

edited

Loading

JekaMas commented Oct 14, 2022 •

edited

Loading