Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[INFRA] Add SonarCloud analysis on CI build #906

Merged
merged 29 commits into from
Nov 30, 2020

Conversation

csouchet
Copy link
Member

@csouchet csouchet commented Nov 25, 2020

Add configuration to run a SonarCloud analysis on master branch and pull requests

Closes #823

@csouchet csouchet force-pushed the 823-Add_code_coverage_public_reports branch from ae1c2b0 to 3cde2a9 Compare November 25, 2020 19:15
.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -49,12 +50,12 @@ jobs:
- name: Install dependencies
run: npm ci
- name: Lint check
run: npm run lint-check
run: npm run lint-check -- -f json -o build/eslint-reporter.json
Copy link
Member

@tbouffard tbouffard Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ why is this needed? if the eslint check fails, the build fails so no other steps are triggered including sonar, so this seems useless

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This generates the Eslint report, and we need it for Sonar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you want to say.
If there is a error we don't analyze with Sonar, and if there is no error, sonar display anything. so it's useless.

Maybe, we should failed the job if the lint failed, only at the Sonar step end.

- name: Build Application
run: npm run build
- name: Test Application
id: 'test_unit'
run: npm run test:unit
run: npm run test:unit ${{ matrix.os.coverage }}
Copy link
Member

@tbouffard tbouffard Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good way to configure the step according to the os! I am not this is the best name, for now, it is ok (I don't have any best proposal 😸)

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

# This is the name and version displayed in the SonarCloud UI.
sonar.projectName=bpmn-visualization
sonar.projectVersion=0.8.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ we have to make the version consistent with what we have in the package.json
We should automate this on version changes: we can do it in another PR, but as soon as possible, otherwise this will become a mess and I don't want we to have to manage this manually.

Copy link
Member Author

@csouchet csouchet Nov 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked myself the same.
We need to change the version on the package.json, because it's corresponding to the last tag. But, here, we are on delopment on the next.
Maybe on the tag workflow, we need to change the current version, after the tag.

Ok to do that on another PR 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern was about the version sync, but yes we can also address the version that stays with the tagged version.
Some projects (I didn't remember which one, but it was a big like jest) use x.y.z-post version after release, we could do the same

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can manage it with #909

@@ -103,6 +103,7 @@
"jest-html-reporter": "^3.3.0",
"jest-image-snapshot": "^4.2.0",
"jest-puppeteer": "^4.4.0",
"jest-sonar": "^0.2.11",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ do you know if this has perf impact on our local dev i.e if you have seen processing time increase when running tests?
I will do some checks on my side

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't check yet.

Copy link
Member Author

@csouchet csouchet Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ubuntu

Before jest-sonar integration

  • Unit test: 22s
  • E2e test: 2m 22s

After jest-sonar integration

  • Unit test: 26s
  • E2e test: 2m 26s

MacOS

Before jest-sonar integration

  • Unit test: 28s
  • E2e test: 3m 32s

After jest-sonar integration

  • Unit test: 25s
  • E2e test: 3m 22s

Windows

Before jest-sonar integration

  • Unit test: 31s
  • E2e test: 3m 32s

After jest-sonar integration

  • Unit test: 38s
  • E2e test: 3m 59s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for the figures
It looks ok

@csouchet csouchet force-pushed the 823-Add_code_coverage_public_reports branch 3 times, most recently from 8c0928d to 853bdf2 Compare November 26, 2020 15:58
@tbouffard tbouffard changed the title [INFRA] Add the code coverage public reports [INFRA] Add SonarCloud analysis on CI build Nov 26, 2020
@csouchet csouchet force-pushed the 823-Add_code_coverage_public_reports branch from 00ea52a to ee873ba Compare November 26, 2020 18:32
.github/workflows/build.yml Outdated Show resolved Hide resolved
@@ -14,6 +14,9 @@
<a href="https://github.com/process-analytics/bpmn-visualization-js/actions">
<img alt="Build" src="https://github.com/process-analytics/bpmn-visualization-js/workflows/Build/badge.svg">
</a>
<a href="https://sonarcloud.io/dashboard?id=process-analytics_bpmn-visualization-js">
<img alt="Coverage" src="https://sonarcloud.io/api/project_badges/measure?project=process-analytics_bpmn-visualization-js&metric=coverage">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the badge

image

@csouchet csouchet marked this pull request as ready for review November 27, 2020 10:11
@csouchet csouchet marked this pull request as draft November 27, 2020 10:42
@csouchet
Copy link
Member Author

I need to fix the ignored tests on test:e2e:coverage, because the performance tests run :(

@csouchet csouchet marked this pull request as ready for review November 27, 2020 10:53
@@ -62,11 +62,11 @@
"lint-check": "tsc --noEmit && eslint \"*/**/*.{js,ts,tsx}\" NOTICE --max-warnings 0",
"test": "run-s test:unit test:e2e",
"test:unit": "jest --runInBand --config=./test/unit/jest.config.js",
"test:unit:coverage": "jest --runInBand --config=./test/unit/jest.config.js --coverage",
"test:unit:watch": "jest --runInBand --config=./test/unit/jest.config.js --coverage --watchAll",
"test:unit:coverage": "npm run test:unit -- --coverage",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ did you try run-s instead of npm (which probably fork a new process). This applies to all scripts that have been chandeg to update another script.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think to do that ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's adapted to what we want to do.
Here we just want to run the same configuration as for the unit test, but with one more paramter (for the coverage).

I tried with run-s. It doesn't work. After the documentation reading of run-s, it's more to run mutliple scripts (it's not what we do).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let's keep it

"test:unit:coverage": "jest --runInBand --config=./test/unit/jest.config.js --coverage",
"test:unit:watch": "jest --runInBand --config=./test/unit/jest.config.js --coverage --watchAll",
"test:unit:coverage": "npm run test:unit -- --coverage",
"test:unit:watch": "npm run test:unit:coverage -- --watchAll",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ behaviour change
I don't use this command (nor know why we created it) so I won't be impacted.
If @aibcmars is ok with this change, let's do it (check the doc if there is any mention to it)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the coverage was already run as part of the 'watch', so there is no behaviour change.

Comment on lines -30 to -37
coverageThreshold: {
global: {
branches: 80,
functions: 80,
lines: 80,
statements: 80,
},
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ I guess this disable the 'fail build' detection when we are under the threshold?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes ^^

Copy link
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@aibcmars can you check #906 (comment) and decide if this is ok to you?

@csouchet csouchet force-pushed the 823-Add_code_coverage_public_reports branch from 3e80f0c to ec18173 Compare November 30, 2020 08:49
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@csouchet csouchet merged commit c06ab22 into master Nov 30, 2020
@csouchet csouchet deleted the 823-Add_code_coverage_public_reports branch November 30, 2020 09:59
@tbouffard tbouffard added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[INFRA] Add the code coverage public reports
2 participants