Review automated source code: Với 4 bước để "chạm" tới chất lượng

#review-code #best-practices #review-checklist

Trần ViễnJuly 12, 2021

Review automated source code: Với 4 bước để "chạm" tới chất lượng

Xem nhanh

Đôi khi, thực hiện task review source code nói chung và e2e automated source code nói riêng, chúng ta loay hoay không rõ review như thế nào là đủ? Khai thác những điểm nào của test code để tự tin rằng mình đã review có “tâm lẫn tầm”?
Review source code có lẽ là task không định lượng dễ dàng như implement featured products. Nếu team chúng ta không có sự thống nhất, cùng “mindset” thì đôi khi reviewing trở nên áp lực và thậm chí dẫn đến những cuộc tranh luận tốn kém thời gian hơn sự cần thiết.
Bài viết hôm nay chúng ta sẽ khai thác từ mindset cho đến best practice giúp chúng ta định lượng task review trước khi bắt tay review.

Review checklist

Trước khi bắt tay vào đọc bài viết này, tôi hy vọng tất cả chúng ta nên hiểu rằng

Review source code là để nâng cao chất lượng source code và cùng nhau học hỏi và thấu hiểu nhau hơn

Định lượng những điểm cần review giúp chúng ta hình dung những gì cần làm và điều đó giúp chúng ta nhất quán trong tất cả lần review automated source code. Bên dưới, chúng ta xem qua một số mục chính mà tôi thường áp dụng trong quá trình review:

#1: Source code được format

Bạn có thể phát hiện bằng mắt bằng cách đọc source code hoặc dùng tool. Phần lớn mỗi công ty sẽ có những công cụ để format mã nguồn cho thống nhất, chẳng hạn: Prettier và cũng có thể một số IDE trả phí có tính hợp sẵn chức năng này như PhpStorm.

Dù bằng cách nào, nhanh hay chậm, reviewer cố gắng kiểm tra xem liệu source code có passed các điểm như: số dòng của class, số dòng của function, số ký tự trên dòng, khoảng trắng, indent code. v.v.

Hãy để máy tính với những công cụ phổ biến như Prettier và ESLint làm công việc phân tích static code hay kiểm tra code style. Reviewer tốn khoản 1 phút để đảm bảo rằng CI “xanh”. Như hình 1.0 mục 1, 2 CI cho chúng ta kết quả check

Hình 1.0: CI kiểm tra và cho ra kết quả passed về formatting code

#2: Automated source code có độ tin cậy (reliability) cao?

Mọi nỗ lực hoàn thành implement có lẽ sẽ suy yếu rất nhiều nếu test code đó không thể thực thi và hoàn thành kịch bản test một cách chính xác tại môi trường reviewer. Mỗi một loại test thường có một đặc tả khác nhau. Đôi khi developer không check hết ở môi trường, ví dụ: Mac OS, Linux hoặc chỉ check ở môi trường đang implement của developer.
Nên để test-code high-stable thì check:

Chạy n lần để đảm bảo đủ sự tin cậy (n được cả team thống nhất trước). Ví dụ: Cybozu Vietnam thống nhất chạy 3 lần, tối thiểu ở môi trường Selenium-grid docker, selenium-standalone service
Đảm bảo test code được chạy trên tất cả môi trường Windows, Mac, hay IOS hay Android,.v.v. theo yêu cầu của test case. Như mục 4 Hình 2.0 bên dưới thể hiện môi trường chạy
Chạy thời điểm nào là hợp lý: Những e2e test case có liên quan thời gian. Ví dụ Add new regular appointment | Cybozu Garoon, có đoạn thời gian dẫn đến test case failed, như cuối ngày (23:59:00), cuối tuần, cuối tháng,… do đó chúng ta nên đảm bảo những tình huống này đã xem xét và đánh giá passed
Chạy mỗi test spec tốn bao lâu thì chấp nhận được: Chạy passed nhưng nếu quá lâu dẫn đến hệ thống test report quá lâu (đi ngược lại lý tưởng automation testing là cho ra kết quả nhanh) thì cũng cân nhắc và feedback. Ví dụ: mỗi e2e test case chạy mất 5 phút đồng nghĩa toàn bộ hệ thống dù tiến hành parallel test cỡ nào đi nữa thì 5 phút vẫn là thời gian tối thiểu. Như mục 6 Hình 2.0 bên dưới thể hiện thời gian chạy
PR của test spec được đánh giá là tin cậy cao sẽ luôn cho ra một kết quả với cùng điều kiện input trong mọi ngữ cảnh. Như mục 5 Hình 2.0 bên dưới thể hiện trường hợp passed, mục 7 Hình 3.0 bên dưới thể hiện trường hợp failed

Hình 2.0: Reviewer sẽ kiểm tra các điểm test code tại môi trường của mình và trường hợp passed

Hình 3.0: Reviewer sẽ kiểm tra test code tại môi trường của mình và trường hợp failed

#3: Tuân thủ coding standards để source code tăng cường tính Maintainability

Automated testing project cũng là một dự án phần mềm do đó cần tuân thủ coding standards mà chúng ta đã thống nhất sử dụng ngay từ đầu. Việc tuân thủ sẽ mang nhiều lợi ích như: Source code nhất quán và ít rủi ro ở giai đoạn maintain. Và nó giúp người mới dễ dàng tiếp cận source code để tiếp tục đóng góp vào nó

Việc developers tuân thủ coding standards cho từng dòng code là bắt buộc nhưng khi review chúng ta kiểm tra xem liệu có chỗ nào bất tuân coding standards mà project following hay không:

Thông thường (Readability, Testability, Debuggability, Configurability)

Với điểm này, chúng ta xem tên biến (Pascal, CamelCase, snake case v.v.), tên hàm, tên class, tên file, cấu trúc dữ liệu sử dụng (array, enum, v.v. cho những giá trị tương tự), comment source code. v.v.
Một ví dụ đặt tên biến KHÔNG đúng với coding rules:

Copy
```
  constructor(addressInfo) {
    this._addressInfo = addressInfo;
    this._usersId = []; // không hợp lệ, chúng ta nên áp dụng rule: Array/list variables rule
  }
  
  addAddress () {
    let allow = true; // Không hợp lệ, chúng ta nên áp dụng rule: Avoid generic names
   // ...
  }
}
```
Tên biến ĐÚNG đúng với coding rules:

Copy
```
export default class AddAddress {
  constructor(addressInfo) {
    this._addressInfo = addressInfo;
    this._userIdList = [];
  }
  
  addAddress () {
    let allowAccess = true;
    // ...
  }
}
```
- [Readability] Self-explanatory - Code tự giải thích nó làm gì và hạn chế tối đa để lại comment trong source code. Để có thể hiểu rõ hơn điều này, bạn có thể đọc bài viết này để có nhiều thông tin hơn
- [Reusability] Source code có duplicate ở đâu trong project hay không? Nói cách khác có tuân thủ DRY principle?
- Vị trí phân bổ file vật lý có đúng vai trò của file trong module, project có phù hợp với structure project tuân theo. Để có thể hiểu rõ hơn điều này, bạn có thể đọc bài viết này để có nhiều thông tin hơn
- Những giá trị thuộc về configuration có được khai báo ở config file chuyên dụng trong project không? webdriverio.conf.js , e2e.setting.global.js, package.json điều này giúp code không thay đổi khi chúng ta thay đổi giá trị thường xuyên.
  Ví dụ chúng ta có một file lưu trữ những giá trị sử dụng trong e2e project: e2e.setting.global.js và được gom nhóm theo vai trò hoặc phạm vi sử dụng, wdioConfig bên dưới là một ví dụ
  
  Copy
```
wdioConfig: {
  // The number of process runs parallel test
  numberOfProcess:       process.env.NUMBER_OF_PROCESS || 1,

  // Does not run the test-spec specified on this
  excludeSpecs:          process.env.EXCLUDE_SPECS ||[],

  // Default level is 'silent', in this case the WebdriverIO framework does not generate log messages when running
  logLevel:              process.env.LOG_LEVEL || 'silent',

  // Try to re-run the hooks of the test-case to n times before to report failed this
  retryNumber:           process.env.NUMBER_RETRIES || 0,

  // When the test-spec file is failed that will retry n time
  specFileNumberRetries: process.env.SPEC_FILE_NUMBER_RETRIES || 0,
},
//...
```
- [Usability] Nếu người khác dùng source code này để phát triển thêm, mở rộng thì có dễ dàng?
  
  Ngoài những điểm kể trên mang tính chất chung chung cho tất cả source code Cybozu Vietnam cũng có thêm một số điểm dành riêng cho testing code như bên dưới:
- Về test flow
  - Tên file test flow: Kết thúc với *.flow.js ví dụ adding-regular-appointment.flow.js. Có bắt đầu là một động từ thêm ing (tên tiếng Anh). Có đủ ngữ nghĩa hay không (tên có toát lên nội dung test flow đảm nhiệm?) như ví dụ phía trước adding-regular-appointment
  - Có được tổ chức đúng nơi không? Đúng thường là thư mục tên test-flows của từng application mà e2e testing triển khai. Ví dụ:
    
    Copy
```
e2e
|---mobile-view
|   |---src
|   |   |---schedule
|   |   |   |---test-flows
|   |   |   |   |---adding-regular-appointment.flow.js
```
  - Class của test flow có constructor nhận props đủ để các test-flows methods hoạt động hiệu quả? Ví dụ bên dưới
    
    Copy
```
export default class AddingRegularAppointment {
    constructor(appointmentData) {
        this._appointmentData = appointmentData;
    }
    // ...
}
```
  - Tất cả test flow methods trong test flow class triển khai có liên quan mật thiết hoặc bổ trợ cho tên class cũng như filename test flow không. Ví dụ addingRegularAppointment bên dưới
    
    Copy
```
import { PersonalWeekPage, RegularAddPage, ViewPage } from '../pages';

export default class AddingRegularAppointment {
    constructor(appointmentData) {
        this._appointmentData = appointmentData;
    }

    addRegularAppointment() {
        PersonalWeekPage.gotoMobileIndexPage();
        PersonalWeekPage.clickOnNewAppointmentLink();
        // ...  
        RegularAddPage.clickOnAddButton();

        return this;
    }
}
```
  - Về test flow methods chúng ta đảm bảo tất cả chúng không có bất kỳ input parameters nào; đảm bảo JSDOC được viết đầy đủ hoặc chúng không gây ra bất kỳ sự nhầm lẫn nào dù ít nhất ở tên flow method
  - Test flows có giữ được lý tưởng vốn để nó sinh ra là: Tiếp cận theo hướng đối tượng hóa của sự kiện trong end-to-end testing (addingXXX, deletingXYZ, v.v.)
  - Test flow class, nó có được niêm yết test flow này với facade/service để các test spec tìm kiếm và sử dụng hay không? Thông thường mỗi module sẽ có một facade instance tên schedule/test-flows/index.js
    
    Copy
```
import AddingRegularAppointmentFlow from '#mobile-view/schedule/test-flows/adding-regular-appointment.flow';

/**
 *
 * @param {Object} appointmentData
 * @returns {AddingRegularAppointment}
 */
export const addRegularAppointment = (appointmentData) => {
    return new AddingRegularAppointmentFlow(
        appointmentData
    ).addRegularAppointment();
};
```
- Về test spec
  
  Tên test spec có phản ánh tổng thể nhiệm vụ mà nó sẽ testing với một hoặc vài test case bên trong nó. Ví dụ test-spec ĐÚNG: test-specs/creating-regular-appointment-include-facility.spec.js
  
  Copy
```
describe('Compose a message', () => {
    before('Sign-in to system', () => {
        login(sender);
    });

    before('Compose a message', () => {
        composeMessage(messageObject);
    });

    beforeEach('Sign in by receiving message', () => {
        login(receiver);
    });

    it('should be receive a message by account in To field', () => {
        openMessageItemInInboxFolder(messageObject);
        verifyOpenedMessageCorrectly(messageObject);
    });

    after('Delete the message added after testing', () => {
        deleteMessage(messageObject);
    });
})
```
  Các hooks được sử dụng đúng với nguyên lý và đảm bảo tính đơn nhiệm ở mỗi hooks
  Ví dụ implement ĐÚNG bên dưới
  
  Copy
```
describe('Compose a message', () => {
    before('Sign-in to system', () => {
        login(sender);
    });

    before('Compose a message', () => {
        composeMessage(messageObject);
    });
    // ...
})
```
- Về page object
  - Tên file có tuân thủ URI và dùng gạch ngang (dashes) để nối các từ hay không? Và kết thúc với .po.js*
  - Action methods đảm bảo có áp dụng method changing
  - Action methods đảm bảo JSDOC được viết đầy đủ hoặc chúng không gây ra bất kỳ sự nhầm lẫn nào dù ít nhất
  - [Debuggability] Đảm bảo log message được implement và implement hiệu quả (hướng tới ngữ nghĩa, thể hiện test data)
Copy
```
// ...
inputTitleTexbox(title) {
    element(this._locators.titleLocator).setValue(title);
    logger.debug(`input the title textbox with the: "${title}"`);
    
    return this;
}
```
- Locator có theo độ ưu tiên hay không (locator strategy)? Id, CSS Query Selector, XPath, JS Function
Copy
```
<input name="_account" class="logintextfield" maxlength="100" value="" title="login name" onkeypress="return event.keyCode != 13;" type="text">
```
- Locators/selectors - Định vị Web element trên trang
  Cũng rất khó để yêu cầu developer sử dụng XPath hay Css Selector hay smart locator trong quá trình review. Mỗi testing framework hỗ trợ các kỹ thuật định vị Web element khác nhau nên khi triển khai chúng ta áp dụng theo khuyến cáo của testing framework đó.
  Chung quy lại, khi review testing code ở điểm này, chúng ta đánh giá xem liệu locator/selector này được chỉ định có hướng đến độ dễ hiểu hay không?
  Ví dụ chúng ta cần viết action method trên thư mục Sent item của màn hình Message module, giao diện như Hình 4.0 bên dưới
Hình 4.0: Ưu tiên lựa chọn locator hướng đến sự dễ hiểu nghĩa thay vì hướng đến kỹ thuật

Ví dụ: locator KHÔNG hướng đến sự dễ hiểu

Copy
```
import { element } from '#e2e-core/wdio-wrapper/element';

class Index {
    constructor(locators) {
        this._locators = locators;
    }
   // ...
    clickOnSentItemLink() {
        element(this._locators.sentItemLink).click();

        return this;
    }
}

module.exports = new Index({
  sentItemLink: '#deepth_0_98',
});
```
Locator cho Web element có hướng đến sự dễ hiểu

Copy
```
import { element } from '#e2e-core/wdio-wrapper/element';

class Index {
    constructor(locators) {
        this._locators = locators;
    }
   // ...
    clickOnSentItemLink() {
        element(this._locators.sentItemLink).click();

        return this;
    }
}

module.exports = new Index({
  sentItemLink: 'span=Sent items',
});
```

#4: Áp dụng “OOAD” Principles phù hợp

Phần lớn developer vận dụng OOP hiệu quả ở môi trường làm việc, thật tuyệt vời! Những kiến thức này là nền tảng giúp developer bước dài trong sự nghiệp.
Nhưng thực tế với OOP không phải "chiếc đũa thần" không thể nào chỉ dựa vào đó mà xây dựng project lớn và có thể mở rộng hiệu quả.
Do đó nhiều nguyên lý (principles) và kiểu thiết kế mẫu (design patterns) ra đời giúp chúng ta xây dựng software project lớn hiệu quả. Trong phạm vi bài viết này, tôi xin phép trích lược một số nguyên lý và cố gắng diễn đạt theo cách hiểu biết riêng mình và đã áp dụng product code và testing code.

Khi review source code chúng ta cố gắng kiểm tra xem:

Liệu source code phát sinh mới có áp dụng được nguyên lý nào bên dưới không?
Và nguyên lý đang áp dụng có phù hợp hay không?

Nguyên lý đơn nhiệm (Single Responsibility Principle)

Mỗi module, mỗi class, mỗi function, mỗi biến: Chúng chỉ nên đảm nhiệm MỘT NHIỆM VỤ duy nhất và cũng có một lý do duy nhất để chúng ta thay đổi nó.
Ví dụ bên dưới trong Garoon automated e2e testing chúng ta khai báo một class CommentAdd với một nhiệm vụ duy nhất là Add Comment. Đồng thời trong class này chúng ta cũng implement function inputComment(content) mà nó có duy nhất nhiệm vụ input comment

Copy

import { element } from '#e2e-core/wdio-wrapper/element';

class CommentAdd {
    constructor(locators) {
        this._locators = locators;
    }
    /**
     *
     * @param {string} content
     * @returns {CommentAdd}
     */
    inputComment(content) {
        element(this._locators.commentDataLocator).setValue(content);

        return this;
    }
  // ...
}
module.exports = new CommentAdd({
    commentDataLocator: '//textarea[@name="comment_data"]',
});

Dễ mở rộng và khó chỉnh sửa (Open Closed Principle)

Dễ dàng để mở rộng chức năng, module, và function nhưng hạn chế chỉnh sửa code đã tồn tại trước đó.
Với cách tổ chức automated source code của e2e testing có dễ dàng cho việc thêm mới test spec mới không? là điểm chúng ta cần xem xét khi review
Liskov substitutability principle

Xác định rõ liệu có cần thiết và có thật sự kế thừa tốt hơn không kế thừa hay không?
Ví dụ kinh điển trong lập trình là: Chim cánh cụt “thoạt nghĩ cũng là chim” nhưng không nên kế thừa lớp “Chim” nếu lớp Chim đã có behavior “bay” :), như vậy kế thừa trong trường hợp này thì không nên

Copy
```
class Chim {
    bay() {
        Console.log('Bay bằng cánh');
    }
}

class DaiBang extends Chim {
    anMoi() {
        console.log('Đại bàng ăn thịt');
    }
}

class CanhCut extends Chim {
// ...
}
```
Lưu ý: Code prototype trên chỉ minh họa cho việc kế thừa thái hóa (chi tiết về nguyên lý này xin tìm hiểu ở một chuyên trang khác)
Interface segregation

Xác định và chia một interface lớn thành nhiều interface nhỏ hơn với những ràng buộc dễ đáp ứng ở class. Điều này tăng tính bảo trì và thuận tiện trong quá trình implement class
Dependency Injection

Thay vì “hardcode” các mối phụ thuộc (dependencies) ở class, component, function ở “body”, chúng ta cố gắng “tiêm” những mối phụ thuộc này bằng những kỹ thuật phổ biến (design patterns) giúp tăng cường tính testable source code/testing code

Kết

Review source code nói chung và testing code nói riêng không những là hoạt động tư duy thuần túy mà còn vận dụng một chút “trí tuệ cảm xúc” giúp việc review thực sự mang tính xây dựng và chất lượng. Ban đầu áp dụng các điểm gợi ý ở trên có thể tốn thời gian, cảm giác gò bó nhưng bù lại chúng ta sẽ thực hiện việc review tất cả task có tính quy cũ từ đó mang đến những dòng code chất lượng trước khi merge vào codebase.

Hơn thế nữa chúng ta có thể tận dụng task review này để “nâng tầm” tech-skill và soft-skill. Vì mọi người đều có thể cho chúng ta điều để học hỏi phải không nào? Dù họ 1 năm nay 10 năm kinh nghiệm.
Tùy vào môi trường và công ty mà có những hoạt động review khác nhau, tiêu chí review khác nhau, v.v. Ví dụ: Tại Cybozu Vietnam những task phát sinh pull request được lập trình theo hình thức Mob programming hoặc pair programing thì KHÔNG review