Test Development Patterns and Anti-Patterns
Have you ever wonder what would be the difference between the code written by a Developer vs Quality Assurance(QA) Engineer?
Test development, similar to software development we need to have design patterns in order for us solve the recurring problems such as unreadable code, inconsistency coding patterns, high efforts of maintaining the test code, etc.
Let’s share across the best coding practices, DOs and DONTs during Test Development in this article.
Code Review Practices
Code Review process is critical in test development as it helps to maintain the consistent patterns, minimise the errors, ensuring QA written the “right” test code.
DOs
- Use Merge Request Template
By having a MR template, it could be used as a checklist when QA submitting their MR for a code review. It’s a good way to enforce ourselves to do self-code review and check everything. Refer article below for more details:
2. Review your code first
Without this, reviews will have too many comments that could be avoided at the first place such as: spelling errors, formatting and reviewer might missed out the more important things. Try to avoid such:
- Anti-patterns
- Formatting/spelling mistakes
- Ambiguous code and comments; missing and incorrect comment; missing asserts
- Too many changes that make review process complicated (like simultaneous refactoring)
3. Test your code
Code is expected to be working with all relevant tests passing. Review process is not designed to verify that the code works. So, it’s time for QA to assure our own test code!
4. Clearly indicate the goal of the review
Quite often there is a need to do preliminary review, when only particular ideas are being offered for review, such as a big test code refactoring, AT for a big feature, etc. In that case it is very important to let reviewers know that the code is only a draft and only particular types of comments are expected. WIP/Draft in a MR subject helps.
5. Focus on the code itself
It’s totally understandable that sometimes we might felt code review could be a bit bitter(or actually sweet), as your code is challenged by someone else and it’s not a good feeling. But let us just focus on the code itself.
When we are being challenged, please keep in mind that:
Say, if Tom is coding something by following the patterns of some existing code from Jerry, and Peter, the code reviewer challenges Tom: “Hey Tom, your code has a problem”, then Tom:
- Should not: complaint that: “Hey, it’s the same or similar code copied from Jerry, why don’t you challenge Jerry but me?”
- Should (after understanding the issues): “Oh ya it’s correct, it’s not a good solution, let me inform Jerry or help him to his part too.”
Coding practices — DOs
Use composition in place of spreading the same properties everywhere
Anti-pattern
//InitPartConsumer ...
func InitPartitionConsumer(consumer sarama.Consumer, topic string, partition int, offset int64) (partconsumer sarama.PartitionConsumer, err error) {
// do something
}func GetKafkaMsgFromMultiPartitions(addr, topic string, maxPartNum int, offset int64, timeout int) (result []string) {
// do something
}
What’s wrong: the topic and addr are everywhere
How to fix: Use composition
type KafkaConn struct {
Topic string
Addr string
Timeout int
Offset int64
}
func (k *KafkaConn) InitPartitionConsumer(consumer sarama.Consumer) (partConsumer sarama.PartitionConsumer, err error) {
// do something
}
func (k *KafkaConn) GetKafkaMsgFromMultiPartitions(maxPartNum int) (result []string) {
// do something
}Don’t you feel it’s much cleaner and comfortable?
Use ‘require’ function to reiterate an array
Anti-pattern
actual := search.Send()
assert.Equal(t, int32(http.StatusOK), actual.Status)
for _, s := range actual.Groups {
for _, k := range s.GetData() {
if k.GetBubbleTeaId() == ExpBubbleTeaId {
assert.Equal(t, ExpBubbleId, k.GetBubbleId())
assert.Equal(t, ExpSugarLevel, k.GetSugarLevel())
assert.Equal(t, ExpDeliveryId, k.GetDeliveryId())
}
}
}What’s wrong: If actual.Groups is [], the assert will not be exercised
How To Fix: Use require library
github.com/stretchr/testify/require
require.NotEmpty(t, k.GetBubbleTeaId())
for _, s := range actual.Groups {
for _, k := range s.GetData() {
assert.Equal(t, ExpBubbleId, k.GetBubbleId())
assert.Equal(t, ExpSugarLevel, k.GetSugarLevel())
assert.Equal(t, ExpDeliveryId, k.GetDeliveryId())
}
}Compute results ahead of time
Anti-pattern
for _, actual := range actualResp.GetBbt() {
var validBbtID = regexp.MustCompile(`^[a-z]+\[[0-9]+\]$`)
// do something
}What’s wrong: Pattern compile operation is invoked multiple times, produces the same result. Every invocation takes execution time
How to fix
var validBbtID = regexp.MustCompile(`^[a-z]+\[[0-9]+\]$`)
for _, actual := range actualResp.GetBbt() {
// do something
}Make test code chatty enough
Why? A test should be somewhat chatty, to give a warm fuzzy feeling that it’s actually doing what it supposed to do. If we are going to report the failure reason or root cause, how can we know what does the test is actually doing?
If you are considering to make a test less chatty, let’s put it as an option when the test is running in a normal way. It will be very frustrating when a test crashes unexpectedly and there is no helpful context written to the output before the crash occured.
How can chatty test solve the problem? In general, the tests written by your team are hard to review because they are complex tests, and because they don’t log any details as the tests are executing. Sometimes, to help understand the test code, we can try to run it or even try to insert bugs into the test code to see how does the test can detects them.
When it’s not chatty enough? It does not help teammates or code reviewer to understand how the test executes if it’s totally inscrutable and does not write anything to a log file or write out any files.
Chatty tests are also useful to verify that the test is working as intended. Sometimes, tests can have bugs too! Be chatty enough so that it is obvious for the test to show it is doing what it is supposed to, and to give context in case of an unexpected failure, but don’t be so verbose that you make it difficult to find the useful info when you need it.
Handle async* responses with a lock
Supposed it needs to send 100 concurrent requests to server and assert 1 OKand 99 NOT_OK
Anti-pattern
var (
OK = 0
NOT_OK = 0
)func doSync(bbtId uint64, resultMap *RequestMap, wg *sync.WaitGroup) {
defer wg.Done()
var (
bbtDeliveryId = bbtId
)
req := &bbtProto.SyncDeliveryRequest{
shopId: nil,
bbtDeliveryId: proto.Uint64(bbtDeliveryId),
}
resp := &bbtProto.SyncDeliveryResponse{}
code := common.sendRequest(req, resp, endpoint.SyncDeliveryCMD)
if int32(code) == ErrorOK {
OK++
return
}
if (int32(code) == utils.ErrorTimeout) || (int32(code) == int32(ERROR_LOCK_FAILED)) {
NOT_OK++
return
}
}func Test_delivery_sync_duplicate_task(t *testing.T) {
var (
bbtDeliveryId = uint64(55138390163352)
ConcurrentDuplicatedSyncTask = 20
)
client.Connect()
results := RequestMap{
data: map[string]int{PassFlag: 0, FailFlag: 0},
}
var wg sync.WaitGroup
for i := 0; i < ConcurrentDuplicatedSyncTask; i += 1 {
wg.Add(1)
go doSync(bbtDeliveryId, &results, &wg)
}
wg.Wait()
assert.Equal(t, 1, OK)
assert.Equal(t, 99, NOT_OK)
}
What’s wrong:
- The requests are concurrent, so the responses are also concurrent. Above code will have race issue when handling the OK++ / NOT_OK++. If it has two PASS and two PASS try to do OK++ in the same time, OK will be still +1
- Use a lock to handle responses when requests are concurrent
How to fix
type RequestMap struct {
data map[string]int
sync.RWMutex
}const (
PassFlag = "PASS"
FailFlag = "FAIL"
)func doSync(bbtId uint64, resultMap *RequestMap, wg *sync.WaitGroup) {
defer wg.Done()
var (
bbtDeliveryId = bbtId
)
req := &bbtProto.SyncDeliveryRequest{
shopId: nil,
bbtDeliveryId: proto.Uint64(bbtDeliveryId),
}
resp := &bbtProto.SyncDeliveryResponse{}
code := common.sendRequest(req, resp, endpoint.SyncDeliveryCMD)
if int32(code) == ErrorOK {
resultMap.Lock()
defer resultMap.Unlock()
resultMap.data[PassFlag] += 1
return
}
if (int32(code) == utils.ErrorTimeout) || (int32(code) == int32(ERROR_LOCK_FAILED)) {
resultMap.Lock()
defer resultMap.Unlock()
resultMap.data[FailFlag] += 1
return
}
}func Test_delivery_sync_duplicate_task(t *testing.T) {
var (
bbtDeliveryId = uint64(55138390163352)
ConcurrentDuplicatedSyncTask = 20
)
client.Connect()
results := RequestMap{
data: map[string]int{PassFlag: 0, FailFlag: 0},
}
var wg sync.WaitGroup
for i := 0; i < ConcurrentDuplicatedSyncTask; i += 1 {
wg.Add(1)
go doSync(bbtDeliveryId, &results, &wg)
}
wg.Wait()
assert.Equal(t, 1, OK)
assert.Equal(t, 99, NOT_OK)
}
Use Builder design pattern for a big request
If a request object is big, use Builder design pattern to build it with default values
Anti-pattern
// just use a json here to show how complicated the req is
reqJson := ` {
"query_id": "demo",
"request_info": {
"timestamp": 1593070231,
"request_id": "central.sg2.6jRWx4rXoGQmFmrS75f5Y001"
},
"bubble_tea_info": {
"with_bubble": true,
"sugar_level": 0
},
"item_group_info": {
"tea_id": 9988000,
"total_price": 100000,
"is_regular": true,
"group_id": 1,
"item_infos": [
{
"bbt_id": 1915067,
"cup_size": 0,
"with_toppings": false,
"quantity": 1
}
]
},
"toppings_info": {
"type": "golden_bubble",
"amount": 2
},
"shop_info": {
"seller_id": null,
"shop_id": 367561
},
"delivery_info": {
"self_pickup": false,
"delivery_code": "",
"delivery_addr": "",
}
}`
req := &bubbleTeaProto.GroupBuyBubbleTeaRequest{}
json.Unmarshal([]byte(reqJson), req)What’s wrong:
- It is very difficult to maintain an object in json. If use regular assignment to prepare an object, it is very tedious and prone to error
- The builder pattern is a design pattern designed to provide a flexible solution to various object creation problems in object-oriented programming. The intent of the Builder design pattern is to separate the construction of a complex object from its representation
newBbtInfo := bbt_info.New().WithSugarLevel(defaultSugarLevel).
WithCupSize(defaultCupSize).
WithToppings(defaultToppings)newBbtGroupInfo := bbt_group_info.New().WithDeliveryInfos(append(make([]*bbtProto.DeliveryInfo, 0), newBbtInfo.BuildByDeliveryMethods(deliveryId)))newShopInfo := shop_info.New().WithShopId(defaultShopId).WithSellerId(defaultSellerId)req := builder.NewMenu().WithBbtGroupInfo(newBbtGroupInfo.BuildByDeliveryMethods(deliveryId)).
WithShopInfo(newShopInfo.BuildByDeliveryMethods(deliveryId)).BuildByDeliveryMethods(deliveryId)
Write a checker to assert a big response
Anti-pattern
func Test_bubble_tea_service_delivery_peak_hour(t *testing.T) {
//...
assert.Equal(t, 1234, actual.BbtId)
assert.Equal(t, 5678, actualAccount.DeliveryId)
assert.Equal(t, 2, actualAccount.SugarLevel)
}
func Test_bubble_tea_service_delivery_non_peak_hour(t *testing.T) {
//...
assert.Equal(t, 1234, actual.BbtId)
assert.Equal(t, 5678, actualAccount.DeliveryId)
assert.Equal(t, 2, actualAccount.SugarLevel)
}What’s wrong: The same assert is spread everywhere and the maintainability is low
How to fix:
// in a common class
func (bbt *BBT) Assert(a *bbtProto.Services) (string, bool) {
if bbt.DeliveryId != a.GetDeliveryId() {
return fmt.Sprintf("bbt.DeliveryId. Expected: %d. Actual:%d", bbt.DeliveryId, a.GetDeliveryId()), false
}
if bbt.ShopId != a.GetShopid() {
return fmt.Sprintf("bbt.ShopId. Expected: %d. Actual:%d", bbt.ShopId, a.GetShopid()), false
}
}
func Test_bubble_tea_service_delivery_peak_hour(t *testing.T) {
//...
bbtChecker := &bbtProto.Services{
BbtId: 1234,
DeliveryId: 5678,
SugarLevel: 2,
}
r, b := bbtChecker.Assert(actualBbt)
assert.True(t, b, r)
}
func Test_bubble_tea_service_delivery_non_peak_hour(t *testing.T) {
//...
bbtChecker := &bbtProto.Services{
BbtId: 1234,
DeliveryId: 5678,
SugarLevel: 2,
}
r, b := bbtChecker.Assert(actualBbt)
assert.True(t, b, r)
}Use plain method to CRUD data
Anti-pattern
In some test code, we need to verify if cache or data in DB is inserted after a request is sent. We should avoid using the same approach with product code (dev) to get the cache and then do the assert. It will:
What’s wrong:
It is a testing methodology problem, “Avoid using dev’s product code to test their product code. Write your own utils code (do not over design, make it straightforward as writing test case) to test” — this one has two advantages:
- If using a buggy code to test a buggy code, it may be difficult to find a bug from itself.
- It will make our test repo very huge. Actually, if we just copy/paste codes from product repos, QA test repo will be full of product codes from all of the product repos and it will be extremely huge and complicated.
Making the test repo too complicated to let junior QA lacking programming experience to join the effort.
- The product’s code is full of concurrency, transaction, logging, and performance codes which are totally redundant to QA’s usage. We just need to write a simple client to read/write data. (Remember our goal, to Keep It Simple and Stupid)
How to fix
- For caches, DB operation, Kafka, etc., we just write some “hello-world” CRUD using raw technologies (cache ADD/DEL, etc., DB SQL CRUD…)
Re-format your code to conform to language coding standard
Code conventions are important to programmers for a number of reasons:
- 80% of the lifetime cost of a piece of software (so is a testware) goes to maintenance.
- Hardly any software (so is a testware) is maintained for its whole life by the original author.
- Code conventions improve the readability of the software, allowing new joiners to understand new code more quickly and thoroughly.
Re-format source code, we can use IDE to re-style code to conform i.e. golang code standard.
Goland: option + cmd + L. Code will be automatically reformatted for you.
DONTs
After so many DOs, let’s see what are the DONTs that we can avoid in Test Development.
Copy-paste
Not only for test development, copy-paste bugs are also very common in product code. Just do a copy-paste without enough modification carelessly
Anti-pattern
case bbtProto.CollectionType_SELF_PICKUP:
q = fmt.Sprintf(
"upad:t=%d_uid=%s_bid=%s_type=self_pickup:%s",
alignedT,
fmt.Sprint(CFG.Buyer.UserId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].BbtId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].ItemId))
case bbtProto.CollectionType_DELIVERY:
q = fmt.Sprintf(
"upad:t=%d_uid=%s_bid=%s_type=self_pickup:%s",
alignedT,
fmt.Sprint(CFG.Buyer.UserId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].BbtId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].ItemId))What’s wrong: Should there be a need to change the logic, the change should happen in a few places
case bbtProto.CollectionType_SELF_PICKUP:
q = fmt.Sprintf(
"upad:t=%d_uid=%s_bid=%s_type=self_pickup:%s",
alignedT,
fmt.Sprint(CFG.Buyer.UserId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].BbtId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].ItemId))
case bbtProto.CollectionType_DELIVERY:
q = fmt.Sprintf(
"upad:t=%d_uid=%s_aid=%s_type=delivery:%s",
alignedT,
fmt.Sprint(CFG.Buyer.UserId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].BbtId),
fmt.Sprint(CFG.BbtMap[bbtDataKey].ItemId))Leaving temporary files behind
Anti-pattern
func (k *Consumer) Consume(file string) {
f, _ := os.Create(file)
w := bufio.NewWriter(f)
defer func() {
if err := recover(); err != nil {
f.Close()
return
}
}()What’s wrong: File is not removed
How to fix
func (k *Consumer) Consume(file string) {
f, _ := os.Create(file)
w := bufio.NewWriter(f)
defer func() {
if err := recover(); err != nil {
f.Close()
os.Remove(file)
return
}
}()Meaningless variable names
Anti-pattern
b := bbtPkg.NewBBT(bbtProto.CollectionType_SELF_PICKUP, "PEAK")
c := collectPkg.New().
WithStatus(bbtProto.NonDelivered)
a.UpdateStatus(c)
mn := deduct.New().
WithAccountBalance(totalAmount)
a.UpdateMoneyData(mn)What’s wrong: Unable to tell which variable is for what
How to fix
bbt := bbtPkg.NewBBT(bbtProto.CollectionType_SELF_PICKUP, "PEAK")
collect := collectPkg.New().
WithStatus(bbtProto.NonDelivered)
a.UpdateStatus(collect)
mn := deduct.New().
WithAccountBalance(totalAmount)
a.UpdateMoneyData(mn)Hardcoded sleep
Anti-pattern
s.ResetBbtMenu(newFlavour)
time.Sleep(30 * time.Second) // hardcoded sleep 30s for reset bbt menuWhat’s wrong: Has to wait 30s for each test run
s.ResetBbtMenu(newFlavour)
found, result := index.AwaitResetMenuUntil(newFlavour, fmt.Sprintf(`"promo_menu":"%s"`, promoMenu)) // the function will check reset for the promo_menu in every 1s and timeout is 30s.Hardcoded literals
Anti-pattern
var wg sync.WaitGroup
for i := 0; i < 100; i += 1 {
wg.Add(1)
go doSync(bbtId, &results, &wg)
}
wg.Wait()What’s wrong: Hardcoding number of sync tasks
How to fix
var (
ConcurrentDuplicatedSyncTask = 100
)
// ...
var wg sync.WaitGroup
for i := 0; i < ConcurrentDuplicatedSyncTask; i += 1 {
wg.Add(1)
go doSync(bbtId, &results, &wg)
}
wg.Wait()Using Assert only in test_suites directory
assert should only be used in the test file and test case itself, i.e.test_suites/bbt_delivery_test.go . We should avoid using this in other files as it will abuse the purposes of assert method. If we are intend to have a fail alert when the tests is not executing as expected, it should be failed inside a test case, not other places.
This helps to ensure the failure message is making sense and for ease of debugging.
Avoid panic in tests
Our principle is to try to avoid using panic in test_suites/, because we are expecting test_suites/ has only pass or fail due to business logic issues. On the other hand, panic is technical terminology, we should avoid the combination of business logic and tech handler (i.e. cannot connect to DB) in test_suites/ .
No error handling in tests
Anti patterns
Test_bbt_delivery(t *testing.T) {
records, err := GetXXFromDB()
if err != nil {// this err is unnecessary and make the code dirty
log.Panic(xx)
}
}Avoid such error handling in tests, as this kind of err should be a tech handler but not a business matters.
Writing good tests
DOs
Test your tests
A good test should fail in case of unexpected product behaviour. A test writer should make sure, if non-obvious, that the test really does fail and there is enough information on failure.
How to fix
Perform a modification to cause test to fail
- Environment. For example remove an expected file
- Product itself. For example, hack to return an incorrect value.
- Test. For example, kill a test when it is running and resume
Verify that the output and/or saved artefacts are good enough for failure analysis
Do not forget to restore the original code.
When tests fail, ensure they fail properly
Using random data
Anti-pattern
header := &bbtProto.RequestHeader{}
header.Requestid = proto.String("test")
header.DeliveryId = proto.Int64(12345678)What’s wrong: The request id is very difficult for test debug. Use a timestamp to generate a request id
How to fix
var (
requestId = fmt.Sprintf("requestId-%d", time.Now().Unix())
deliveryId = int64(12345678)
)
req := &bbtProto.DeliveryRequest{
header := &bbtProto.RequestHeader{}
header.Requestid = proto.String(requestId)
header.DeliveryId = proto.Int64(deliveryId)
}Hiding problem details
Anti-pattern
func ReadFileToArray(path string) []string {
file, err := os.Open(path)
if err != nil {
log.Println("unable to open file)
return []string{}
}
defer file.Close()What’s wrong:
- Not reporting file name
- Hiding the inner exception details
How to fix
func ReadFileToArray(path string) []string {
file, err := os.Open(path)
if err != nil {
log.Println("File name: %s", path)
log.Println(err.Error())
return []string{}
}
defer file.Close()Do asserts outside of test case
assert.XXX(t, exp, actual)should be done inside test cases. Should not do assert in a common function
Anti-pattern
func CheckUploadCsvReqWithDb(t *testing.T, id uint64, req *admin.UploadCsvRequest) {
dao := db.CsvDao{
Id: id,
}
actual := dao.RetrieveCsvRecord()
assert.Equal(t, req.GetFileName(), actual.GetFileName())
assert.Equal(t, req.GetFileType(), actual.GetFileType())
assert.Equal(t, req.GetLabelId(), actual.GetLabelId())
//....
}
func Test_001(t *testing.T) {
// do something
CheckUploadCsvReqWithDb(t, 123, req)
}
// ...
func Test_005(t *testing.T) {
// do something
CheckUploadCsvReqWithDb(t, 456, req)
}What’s wrong
- If the case is failing, we have to jump from here or there to navigate which line of code causes this error. E.g,
Test_bbt_delivery_services_peak_hour.go:888We can quickly go to line 888 to navigate the error. It is inside the test case itself. - Otherwise, if, say 5 cases use
CheckUploadCsvReqWithDband fail, 5 cases will fail in the same line and it will cause the error context not comprehensive enough.
How to fix
func CheckUploadCsvReqWithDb(id uint64, req *admin.UploadCsvRequest) (result bool, errMsg string) {
dao := db.CsvDao{
Id: id,
}
actual := dao.RetrieveCsvRecord()
if req.GetFileName() != actual.GetFileName() {
return false, fmt.Sprintf("Expected GetFileName Not Match. Expected GetFileName: %s. Actual GetFileName: %s", req.GetFileName(), actual.GetFileName())
}
if req.GetFileType() != actual.GetFileType() {
return false, fmt.Sprintf("Expected GetFileType Not Match. Expected GetFileType: %d. Actual GetFileType: %d", req.GetFileType(), actual.GetFileType())
}
// ...
func Test_001(t *testing.T) {
// do something
result, errMsg := CheckUploadCsvReqWithDb(resp.GetId(), req)
assert.True(t, result, errMsg)
}
// ...
func Test_005(t *testing.T) {
// do something
result, errMsg := CheckUploadCsvReqWithDb(resp.GetId(), req)
assert.True(t, result, errMsg)}Quick Recap
In this article, we have discussed:
- Code Review Practices
- Coding Practices — DOs & DONTs
By leveraging good test development patterns, it could empower better maintainability, clarity, readability and stability in our automated test cases! Cheers! 🎉
