Mattermost is an open-source cross-platform solution for collaborative communication among teams. In previous posts, we discussed Mattermost’s vision and the overview of its architecture. As we extensively covered the key quality attributes in previous posts, we do not address them here. In this post, we look into the processes put in place to ensure good code quality. We also dive deeper into the analysis of hotspot components and future evolution of the various parts of Mattermost. Finally, we look at the technical debt present in the system in the last section.

Software quality and Integration processes

Our vision for QA is to earn and protect trust in the reliability and usability of Mattermost, and to continue to champion high quality standards as we and our customers grow¹.

To achieve this goal, Mattermost relies on well-defined workflows for the QA team². QA members are usually involved in the PR review process where they review the coverage and E2E tests³. They also establish the test plan and scope of the tests.

For mattermost-server, every PR goes through a number of automated checks before getting merged into the master branch. Continuous integration platforms like CircleCI and GitHub Actions are used to facilitate the creation and execution of these automated checks. Observing the configuration files for these CI tools in the mattermost-server repo revealed that GitHub actions are mostly used for running the code quality analysis and publishing the subsequent reports, while the more important build and test checks are executed on CircleCI.

.github/workflows contains two main code analysis processes:

• CodeQL Analysis: Perform semantic code analysis to discover vulnerabitlies⁴ in code and upload results for later use
• Scorecards analysis: An automated tool that assesses a number of important heuristics (“checks”) associated with software security⁵. Upload files for later use.

Similarly, inspecting .circleci/config.yml shows many checks the PR must pass.

These varied checks and safeguards in place show the strong and robust CI process that is part of the Mattermost development that enables fast development while ensuring minimal faulty code merges.

Test processes

In this section, we look at the test coverage and the rigor of the testing porcesses for mattermost-server.

For every PR that includes new functionalities, it is mandatory to include unit and e2e tests as mentioned by the contribution checklist⁶. Keeping this in mind, the tests can be expected to cover most of the core components well.

In practice, although one can run all tests locally on a developers machine, this takes a lot of time and contributers only write and run tests for their specific contribution. The Go project is set up in a way that any sub-package and individual features can be tested in isolation⁷. In order to run all the tests, Mattermost has set up separate dedicated servers to run tests that are a part of their CI process. Core committers and staff can trigger a test server to test bigger scenarios for any given PR by adding specific labels⁸. Different types of tests (labels) available today are:

• Setup Cloud Test Server: Standard test server using the latest commit
• Setup HA Cloud Test Server: High availability test server
• Setup Cloud + CWS Test Server: Test server that connects to a test Customer Web Server

In conclusion, the testing process is quite robust with flexibility to test at different levels, while ensuring that all features are tested well in multiple scenarios. This prevents developers from shipping changes that break any previous functionality by mistake because of unusual corner cases. This provides more confidence for the users and developers.

Hotspot components and their code quality

Mattermost is a big project, with mattermost-server being the repository that serves Mattermosts’s frontend applications. Having approximately 2,133,569 lines of code (based on the analysis tool CodeScene⁹), mattermost-server has probably faced code issues along the way. Hotspot Components may have been one of the issues, as it can often occur that some files get edited much more frequently and carry much more logic than others. Hotspots are inherently subjects to more code health/structure problems - they do not represent the quality of the whole system. In this section, we shall only discuss a few notable hotspots - an essay covering all of them would be a much longer read.

To analyze hotspots, we used CodeScene code analysis tool. It allows for, among other things, technical debt management and code health analysis. We ran the tool on our fork of mattermost-server. Code issues discussed later in this section will be primarily based on the output of CodeScene.

Hotspots up until today

The following figure shows the hotspot component analysis of the mattermost-server codebase:

In this visualization, the size of the circles gets determined by #LinesOfCode, and colors range from blue to dark red based on the #commits per year. The figure also shows components from Mattermost’s dependencies (vendor/ directory) and components that are automatically generated files - these we shall not discuss.

server.go (89 commits / 1 year, 2318 LOC)

This module is responsible for server bootup - that includes setting up relevant metrics, jobs, email services, telemetry, and, consequently, starting the server. Arguably, this is the most important file of the whole application. To start off, as CodeScene’s analysis reveals, this module has low cohesion. Cohesion is the degree to which the elements inside a module belong together¹⁰. So, high cohesion is desirable since it implies that all functions within the module are related and share the same responsibilities. Low cohesion can be fixed in multiple ways. The most common one is Extract Class method - it implies that one class should have a single responsibility. Once a class gets more responsibilities, it is nice to put separate logic in different classes, and later on, access the newly created classes in the original one.

Other problem of the module is a code smell called Bumpy Rode. It can be characterized as follows:

The Bumpy Road code smell is a function that contains multiple chunks of nested conditional logic. Just like a bumpy road slows down your driving, a bumpy road in code presents an obstacle to comprehension¹¹.

Possible solutions to this issue would involve identifying different responsibilities of the function and applying Extract Function refactoring method (extracting separate responsibilities into separate descriptive functions and calling them in the original function).

Lastly, the module has deep, nested complexity in NewServer and Start functions. This can be seen in the screenshot below:

The image shows the logic for forwarding requests to a different port. While nested conditional logic does the job, the overall readability of the code block suffers greatly. Furthermore, increased cyclomatic complexity opens room for more bugs. To reduce the complexity, one can introduce separate functions for handling some bits of the logic. Additionally, guard clauses can help significantly with cleaning up the code.

config.go (56 commits / 1 year, 3967 LOC)

Having no module documentation, around 200 constants defined on top of the file, and many functions with the same names for different types of inputs, it is very hard to get into the code and quickly grasp everything that is going on within the module. In short, config.go is responsible for setting up the server’s config. With so many different clients, it is no wonder that configurations may be completely different, which inherently defines the complexity of the module. The file has very high cyclomatic complexity in some functions. It is understandable - based on the requirements of the module, it is absolutely not trivial to keep low complexity.

user.go (53 commits / 1 year, 2547 LOC)

This module is responsible for retrieving and creating Mattermost users, changing users’ authentication settings, resetting passwords, etc. While most functions in the module are easy to follow, some suffer from high cyclomatic complexity, like UpdateUser - it has issues with nested conditional logic, similar to the previous figure.

Hotspots of the future

As the product grows, new hotspots can appear. As can be seen on the product roadmap¹², the Mattermost Team seems to be focused on building more integrations and improvements to the user experience in both mobile and web applications. Based on the roadmap, it is hard to predict new future hotspots in mattermost-server repo. Naturally, some of the upcoming features may directly impact the already existing components of mattermost-server. For example, “Custom user groups” feature from the roadmap may impact the aforementioned hotspot user.go. Additionally, “Collapsed reply threads (GA)” may require changes in channel.go (a hotspot module keeping the functions for managing and creating channels). Lastly, as requirements of the system grow and more configuration cases arise, the aforementioned hotspots config.go and server.go will definitely remain subject to change in the future.

Quality culture

There are no PRs marked as priority (critical-urgent, important-longterm or important-soon), which already indicates that mattermost-server is working quite well. Mattermost considers the review process very important. Nothing can be merged without at least one review and a test validation, even for minor PRs that do not have a big impact on the architecture¹³. In some cases, integration tests are not necessary. Smoke tests¹⁴, or even just unit tests, can be sufficient to verify the PRs¹⁵¹⁶.

Mattermost developers and testers often make shared documents and discuss different changes and their impacts¹⁷. Usually, there is a discussion between reviewers and submitter until everyone agrees upon the changes¹⁸, sometimes including the QA team¹⁹. This may lead to new features being added to the original scope of the PR, or in some instances to new PRs being created¹⁹²⁰. Mattermost reviewers prefer clear and concise code to enable everyone to build upon it ¹⁴¹⁵.

Regarding code additions, it is frequent to discuss, both in developer channels and in the reviews, whether some changes should be done in mattermost-server or in the client-side applications²⁰. It is generally preferred to have “messy” changes (when they cannot be avoided) in client-side²⁰, so they are as far away from the backend core as possible. Furthermore, coordinating the open source code with private code from Mattermost enterprise is another challenge that may occur²¹.

Upon new contributions, it is usually preferred to keep consistency with previous code over “optimal” solutions¹⁸. This means the new code may not be the best, but the trade-off for consistency is considered valuable enough as it helps future refactoring. Eventually, big refactoring is carried out at once¹⁴. This trade-off is valuable when teams try to split up the monolithic code of the server into smaller components¹⁴.

There are also tools to help managing the PRs. A Mattermost bot can add some warnings and labels to the PRs based on their scope of impact and or their contributors. For instance, when a PR is impacting the CLI command tool for Mattermost, the bot adds a warning to be careful that the new code does not break any core functionality²². There can also be a “do-not-merge” label applied to a PR if it is not linked to any release note¹³.

Technical debt

Mattermost has been an open-source project since 2015. Any software accumulates technical debt as it grows, and mattermost-server is not an exception. The technical debts can be found in the issues of the mattermost-server repository tagged as Area/Technical Debt²³. Some of these date back to as far as early 2019, showing about 3 years worth of technical debt. Further inspection reveals that there are mainly 4 categories they fall into.

• Migrating tests to TypeScript
• Adding E2E tests

Some client side issues are also tracked in the mattermost-server repository leading to JavaScript related tech debts

• Moving parameters to redux-store
• Refactor legacy code to use CSS variables

In addition to this, scanning the codebase for “TODO"s gives a bunch of hits (59 matches across 34 files) which are definitive signs of tech debts building up.

Considering Martin Fowler’s analysis on the crufty nature of technical debt²⁴, it is completely okay to have debt in areas of the code that are not actively changed. The downsides of technical debt are increasingly felt when they exist in the hotspot components. With these considerations, it seems logical that some volume of tech debts are left over since a long time as they are outside the hotspot components. Regardless, Mattermost could use more help from the community to tackle and minimize these technical debts.

---

1. Mattermost Handbook. R&D, Quality. https://handbook.mattermost.com/operations/research-and-development/quality ↩︎

2. Mattermost Handbook. QA Workflow. https://handbook.mattermost.com/operations/research-and-development/quality/qa-workflow ↩︎

3. Mattermost Developers. Webapp E2E Testing https://developers.mattermost.com/contribute/webapp/e2e/ ↩︎

4. Github. CodeQL. https://codeql.github.com/ ↩︎

5. Open Source Security Foundation. Scorecard. https://github.com/ossf/scorecard ↩︎

6. Mattermost Developers. Contribution Checklist. https://developers.mattermost.com/contribute/getting-started/contribution-checklist/ ↩︎

7. Mattermost Developers. Server Developer Workflow. https://developers.mattermost.com/contribute/server/developer-workflow/#workflow ↩︎

8. Mattermost Developers. Pull Request Test Servers. https://developers.mattermost.com/contribute/getting-started/test-servers/ ↩︎

9. CodeScene. https://codescene.com/ ↩︎

10. GeeksForGeeks. Software Engineering | Coupling and Cohesion. https://www.geeksforgeeks.org/software-engineering-coupling-and-cohesion/ ↩︎

11. Adam Tornhill. The Bumpy Road Code Smell: Measuring Code Complexity by its Shape and Distribution. https://codescene.com/blog/bumpy-road-code-complexity-in-context/ ↩︎

12. Mattermost. Product Direction. https://mattermost.com/roadmap/ ↩︎

13. Replaces SessionLengthSSOInDays with SessionLengthSSOInMinutes. https://github.com/mattermost/mattermost-server/pull/19712 ↩︎

14. Changing from public to private a lot of methods in App layer. https://github.com/mattermost/mattermost-server/pull/18395 ↩︎

15. Implement user access tokens and new roles (server-side). https://github.com/mattermost/mattermost-server/pull/6972 ↩︎

16. Stage 1 of caching layer. Framework and store changes. No behavior changes.. https://github.com/mattermost/mattermost-server/pull/6693 ↩︎

17. Major post list refactor. https://github.com/mattermost/mattermost-server/pull/6501 ↩︎

18. Advanced Permissions Phase 1 Backend. https://github.com/mattermost/mattermost-server/pull/8159 ↩︎

19. Combining consecutive user join/leave system messages to single message and few other changes.. https://github.com/mattermost/mattermost-server/pull/5945 ↩︎

20. Consolidate add/remove/join/leave system message. https://github.com/mattermost/mattermost-server/pull/7473 ↩︎

21. Create App type to carry our state and eliminate global Srv references. https://github.com/mattermost/mattermost-server/pull/7167 ↩︎

22. Add set_online option for creating a post to client4.go. https://github.com/mattermost/mattermost-server/pull/14098 ↩︎

23. Mattermost Server Repository. Technical Debt Issues. https://github.com/mattermost/mattermost-server/labels/Area%2FTechnical%20Debt ↩︎

24. Martin Fowler. Technical Debt. https://www.martinfowler.com/bliki/TechnicalDebt.html ↩︎