Proposal of Static Code Analysis Subgroup for new Acceptance Criteria #73
There are no files selected for viewing
|
There was a problem hiding this comment. Reminder to bump "version 2.0 (ratified 2022-06-10)". |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,7 +20,7 @@ Please see [Before Development](MODULE_EVALUATION_TEMPLATE#before-development) f | |
| 1. Module adheres to Community Code of Conduct | ||
| 1. Module license is compatible with the FOLIO Project | ||
| 1. Module can be included in existing community build processes | ||
| 1. Module has robust and fully automated testing procedure supplied and documented by the development team | ||
|
There was a problem hiding this comment. How can / should the development team provide such a testing procedure? What does fully automated mean in this context? Does it mean that automated tests within the module build must provide 100% coverage? There was a problem hiding this comment. No. 80% code coverage. "Fully automated" is supposed to mean that the tests are automatically carried out upon each Pull Request. There was a problem hiding this comment. i.e. that they are not "manual". |
||
| 1. Module can be deployed in the Community’s reference environments without undue burden | ||
| 1. Module is secure | ||
| 1. Module is multi-tenant | ||
|
|
@@ -58,8 +58,13 @@ Please see [Before Development](MODULE_EVALUATION_TEMPLATE#before-development) f | |
| * _This is not applicable to libraries_ | ||
| * Must not depend on a FOLIO library that has not been approved through the TCR process | ||
| * Gracefully handles the absence of third party systems or related configuration. (3, 5, 12) | ||
| * An automated code scannner hasn't identified any security issues, major code smells or excessive (>3%) duplication (6); and any disabled or intentionally ignored rules/recommendations are reasonably justified. | ||
|
There was a problem hiding this comment. How will this automated code scan be run? (currently, Sonar is run by the centralised build process) If I'm understanding the expanded explanation below, we are stating that any module that uses a language not compatible with Sonar is effectively forced to implement a build within github actions, because there is no other mechanism that can reasonably be used during builds How will evidence of the results be supplied to the TC? (currently the results are published to sonar cloud) There was a problem hiding this comment.
That would be my understanding as well, and I guess would just be the overhead of introducing a new language that's not Sonar-compatible. I'm not clear how feasible that is though, given I think we have a shared build process across modules? So it would have to account for multiple options based on language or something. There was a problem hiding this comment.
effectively: yes There was a problem hiding this comment.
It suffices to give the "metrics" (=characteristic numbers) as mentioned. Those might be supplied in a YAML file, but does not need to be. See the examples on the page Examples+of+Code+Duplication... (we might need to give more examples) There was a problem hiding this comment.
As far as I recall, the Grails/Groovy modules are not Sonar-compatible. Therefore I changed this sentence: "Sonarqube" -> "An automated code scannner" . In the group discussions I think we agreed that it is in the development teams' responsibility to implement automated test (also for the non Sonar-compatible modules). There was a problem hiding this comment.
That was going to be my next question I don't know if the back end modules are using github actions If they are, then I imagine at the very least, the team will need to implement a custom build definition / extensions to use a different static code analysis tool There was a problem hiding this comment.
When I read I don't think of static analysis as a test, rather it is a separate activity to determine some metrics / characteristics of the code, usually to inform discussions about the general quality of the code (rather than if it behaves as intended) @inkuss In this context, does There was a problem hiding this comment.
I think @julianladisch has more details here.
I agree. Maybe "test" is the wrong word here. There was a problem hiding this comment. In the TC we agreed that unit tests and integration tests are not subject of the work of this subgroup. |
||
| * See [Rule Customization](https://dev.folio.org/guides/code-analysis/#rule-customization) details. | ||
| * The code scanner may be Sonar (via Jenkins), github Actions or other. The testing algorithm must be set up, maintained and documented by the development team. The tests need to be reproducible by the Community. | ||
|
There was a problem hiding this comment. I understand the intent of
is to handle new languages not covered by existing testing algorithms. But the statement goes too far, since this criteria applies to all new modules, and any module in an existing language, i.e. Java, would not need anyone, least of all this development team, to "set up, maintain and document" Sonar. Right? Same concern applies to the "values" section edit above. There was a problem hiding this comment.
What is meant by I'm concerned that these changes are mixing together automated testing and static code analysis Personally, I consider those to be separate activities with different purposes There was a problem hiding this comment.
Yes. There was a problem hiding this comment. I asked in the group chat if the other sub group members have a better understanding of what is the scope of this group. In particular, concerning changing the acceptance criteria of automated tests. If we should not find agreement there, I agree that this should be a topic of a Monday TC meeting. There was a problem hiding this comment. I think that Marc has a point, we use SonarQube as a tool to identify code smell, bugs and potential security issues. These are the domain of static code analysis. SonarQube is also used to enforce a standard for code coverage as reported by the coverage tools that are employed for the specific language. This is not, strictly speaking, part of the static code analysis. I do think this is valid, but it was a nuance that was not apparent to myself of the other members (at least as far as our discussions went.) Given that, we should most likely revisit the scope in council to nail down specifically what this group is formed to address. |
||
| * Examples for Code Duplication Detection metrics (i.e. characterstic numbers) can be found [here](https://folio-org.atlassian.net/wiki/spaces/TC/pages/386891824/Examples+of+Code+Duplication+Metrics+and+Code+Smells+Severity) . | ||
| * The metric used for code smells module acceptance must be the highest severity level: | ||
| * Sonarqube uses the severity level _High_. | ||
| * CodeNarc uses the severity level _Critical_. | ||
inkuss marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * Uses [officially supported](https://wiki.folio.org/display/TC/Officially+Supported+Technologies) build tools (3, 5, 13) | ||
| * Unit tests have 80% coverage or greater, and are based on [officially supported technologies](https://wiki.folio.org/display/TC/Officially+Supported+Technologies)[^1] (3, 4) | ||
|
|
||
|
|
@@ -68,7 +73,7 @@ Please see [Before Development](MODULE_EVALUATION_TEMPLATE#before-development) f | |
| Note: Frontend criteria apply to both modules and shared libraries. | ||
|
|
||
| * If provided, End-to-end tests must be written in an [officially supported technology](https://wiki.folio.org/display/TC/Officially+Supported+Technologies)[^1] (3, 4) | ||
| * -_note: If the new module works together with existing modules, the team must provide a result of an integration test. | ||
|
There was a problem hiding this comment. What is the motivation for making this mandatory if a module has any interaction with another module? Given this is within the front end section of the criteria, in practice this means that every front end module will have to implement some set of end to end automated tests (as there is no provision for integration tests for the front end modules IIRC) Is that a reasonable understanding of this expectation? There was a problem hiding this comment. Yes. Julian and I think that it is fair to write integration tests (mandatory) if the module is not stand-alone. If it is not a requirement, it does not even need to be mentioned in the acceptance criteria, we think. There was a problem hiding this comment.
What is meant by My definition of that term in this context is a automated test that demonstrates multiple modules interacting with each other In FOLIO I believe that varies based upon the type of module:
The original criteria was included because we wanted to make sure that if developers chose to write that style of test, then it should be written in the already agreed technologies (and not another tool) There was a problem hiding this comment.
Yes, that fits to what I have in mind. |
||
| * -_note: these tests are defined in https://github.com/folio-org/stripes-testing_ | ||
| * Have i18n support via react-intl and an en.json file with English texts (8) | ||
| * Have WCAG 2.1 AA compliance as measured by a current major version of axe DevTools Chrome Extension (9) | ||
|
|
@@ -92,8 +97,8 @@ Note: Backend criteria apply to modules, shared backend libraries, and edge modu | |
| * All API endpoints protected with appropriate permissions as per the following guidelines and recommendations, e.g. avoid using *.all permissions, all necessary module permissions are assigned, etc. (6) | ||
| * -_note: read more at https://dev.folio.org/guidelines/naming-conventions/ and https://wiki.folio.org/display/DD/Permission+Set+Guidelines_ | ||
| * Module provides reference data (if applicable), e.g. if there is a controlled vocabulary where the module requires at least one value (3, 16) | ||
| * Integration (API) tests must be written in an [officially supported technology](https://wiki.folio.org/display/TC/Officially+Supported+Technologies)[^1] (3, 4) | ||
| * -_note: If the new module works together with existing modules, the team must provide a result of an integration test. | ||
| * -_note: these tests are defined in https://github.com/folio-org/folio-integration-tests_ | ||
| * Data is segregated by tenant at the storage layer (6, 7) | ||
| * The module doesn’t access data in DB schemas other than its own and public (6, 7) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder that after the changes to this file are agreed to, the same should be made to the TEMPLATE file.