Feature/replace karma-typescript by karma #2659

SuperITMan · 2021-04-29T16:16:15Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/NationalBankBelgium/stark/blob/master/CONTRIBUTING.md#-commit-message-guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[X] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Stark-testing is using karma-typescript.

Issue Number: #2653

What is the new behavior?

Using karma + @angular-devkit/build-angular:karma instead of karma-typescript

Does this PR introduce a breaking change?

[X] Yes
[ ] No

Other information

SuperITMan · 2021-04-30T13:06:58Z

Hey @christophercr @nicanac I adapted the test logic in Stark to use karma config from angular and add our custom configuration.

In the meanwhile, I copied stark-testing dependencies into root package.json to be able to run ng test stark-core from root. The direct downside is that we have to maintain stark-testing dependencies manually.
The goal will be to use @nrwl/nx or a similar solution in the future and to centralize dependencies in the root folder.

Could you have a look ? 😊

christophercr

Nice job! Indeed, having a centralized testing logic compatible with NG CLI will always give us better compatibility with the NG ecosystem 😄

I just have some small thoughts about it... see my comments below.

christophercr · 2021-05-03T08:20:23Z

starter/package.json

@@ -127,8 +127,8 @@
    "@angular/platform-server": "^8.2.0",
    "@angular/router": "^8.2.0",
    "@nationalbankbelgium/code-style": "^1.5.0",
-    "@nationalbankbelgium/stark-core": "latest",
-    "@nationalbankbelgium/stark-ui": "latest",
+    "@nationalbankbelgium/stark-core": "file:../dist/packages-dist/stark-core/nationalbankbelgium-stark-core-10.2.0-71462b2f.tgz",


Shouldn't these changes be reverted? Normally the Starter should install the latest from all Stark packages right?

Indeed, you're right. This file should not be changed. Good catch! 👍

christophercr · 2021-05-03T08:21:06Z

starter/package.json

@@ -143,8 +143,8 @@
  },
  "devDependencies": {
    "@compodoc/compodoc": "~1.1.11",
-    "@nationalbankbelgium/stark-build": "latest",
-    "@nationalbankbelgium/stark-testing": "latest",
+    "@nationalbankbelgium/stark-build": "file:../dist/packages-dist/stark-build/nationalbankbelgium-stark-build-10.2.0-71462b2f.tgz",


Shouldn't these changes be reverted? Normally the Starter should install the latest from all Stark packages right?

christophercr · 2021-05-03T08:22:03Z

showcase/tsconfig.spec.json

@@ -1,7 +1,4 @@
 {
  "extends": "./tsconfig.json",
-  "compilerOptions": {
-    "module": "commonjs"


Don't remember exactly why we supported commonjs libs in tests 🤔

christophercr · 2021-05-03T08:25:50Z

combine-packages-coverage.js

-// this ensures that Coveralls can show the list of packages correctly including their files
-for (const fileName of fileNames) {
-	const packageName = fileName.match(/stark-\w*/)[0];
-	const replacements = [{ searchValue: /SF:(.*)(\r|\n)/, replaceValue: `SF:packages/${packageName}/$1$2` }];


Are you sure that with ng test the reports are split by package name? So that the coverage of all stark packages is shown in Coveralls correctly

Actually, since we use ng test stark-core --code-coverage from stark root folder instead of ng test from "packages/stark-core" folder, the paths are correct.

ng test --code-coverage from "packages/stark-core", lcov contains path "SF:src/common/error/error.ts"
ng test stark-core --code-coverage from root, lcov contains path "SF:packages/stark-core/src/common/error/error.ts"

christophercr · 2021-05-03T08:26:55Z

packages/stark-rbac/base.spec.ts

 import "zone.js/dist/zone";
+import "zone.js/dist/zone-testing";


Ah nice! Is it really that simple now?

christophercr · 2021-05-03T08:31:01Z

packages/stark-testing/karma.conf.js

+	browserDisconnectTimeout: 30000,
+
+	// Configuration for coverage-istanbul reporter
+	coverageIstanbulReporter: {


Mmm are you sure that running code coverage also in non-ci environment is fast enough? As far as I remember, this was a bit slower than normal testing without coverage, that's why we did it only for CI...

Wait... I see that the script passes ng test --code-coverage only for CI... so that is basically enabling code coverage only in CI right?

Exactly! 😊

The configuration for coverageIstanbulReporter is there but to enable this reporter you need the --code-coverage flag. It has no effect on ng test without the flag.

christophercr · 2021-05-03T08:37:37Z

package.json

    "cross-env": "^7.0.3",
    "cz-customizable": "^6.3.0",
    "hammerjs": "^2.0.8",
    "husky": "^4.3.8",
+    "jasmine-core": "^3.6.0",
+    "karma": "^5.2.3",


About moving all deps from stark-testing to the root... could we try something else so we can avoid duplication?

What about just adding stark-testing as DevDependency here and link it to the stark-testing folder (no need for a compiled tgz I think because we don't compile anything in that package). That way the deps will be installed at the root... right? 🤔

Ha ha, I didn't think to this since the dependencies are not well maintained after (doing npm i from root after upgrading deps in stark-testing has no impact on installed deps in root).

However, since we are looking for a better solution in the future to manage our monorepo, I think it's better to use your solution and not copy deps from testing into root folder 👍

SuperITMan · 2021-05-03T15:52:09Z

@christophercr I updated my PR based on your remarks. Could you have a look ? 😊

…cies in root folder Stark-Testing dependencies are now installed in Stark root folder instead of being installed in stark-testing folder

…r/devkit:build-angular` Increase test speed thanks to karma + stop use "commonjs" module during test. BREAKING CHANGE: Adapt angular.json, package.json and base.spec.ts files. Check test config in "angular.json": ``` // ... "test": { "builder": "@angular-builders/custom-webpack:karma", "options": { "main": "base.spec.ts", "karmaConfig": "./karma.conf.js", "tsConfig": "tsconfig.spec.json" } } ``` Check tests scripts in "package.json": ``` // BEFORE "test-fast": "karma start", "test-fast:ci": "karma start karma.conf.ci.js", // AFTER "test-fast": "npm run ng test", "test-fast:ci": "cross-env CI=1 npm run ng test --code-coverage", ``` Adapt "base.spec.ts" file as follows: ```typescript "use strict"; import "core-js/es"; import "core-js/proposals/reflect-metadata"; // IE polyfills // See https://developer.mozilla.org/en-US/docs/Web/API/Element/matches#Polyfill /* tslint:disable:no-unbound-method */ if (!Element.prototype.matches) { Element.prototype.matches = (<any>Element.prototype).msMatchesSelector || Element.prototype.webkitMatchesSelector; } /* tslint:enable:no-unbound-method */ // See: https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach#Polyfill if ((<any>window).NodeList && !NodeList.prototype.forEach) { (<any>NodeList.prototype).forEach = Array.prototype.forEach; } /* tslint:disable:no-import-side-effect */ import "zone.js/dist/zone"; import "zone.js/dist/zone-testing"; import "zone.js/dist/long-stack-trace-zone"; /* tslint:enable:no-import-side-effect */ // define global environment variable (used in some places in stark-core and stark-ui) global["ENV"] = "development"; import { getTestBed } from "@angular/core/testing"; import { BrowserDynamicTestingModule, platformBrowserDynamicTesting } from "@angular/platform-browser-dynamic/testing"; // tslint:disable:completed-docs bool-param-default declare const require: { context(path: string, deep?: boolean, filter?: RegExp): { keys(): string[]; <T>(id: string): T; }; }; getTestBed().initTestEnvironment(BrowserDynamicTestingModule, platformBrowserDynamicTesting()); // Then we find all the tests. const context = require.context('./src', true, /\.spec\.ts$/); // And load the modules. context.keys().forEach(context); ```

+ Adapt GitHub Actions script to run starter tests

…age script

sonarqubecloud · 2021-05-05T09:29:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

christophercr · 2021-05-05T09:31:09Z

@SuperITMan btw... where did the Coveralls checks go?

christophercr

Looks great!

Just the remark about Coveralls checks missing 🤔

SuperITMan · 2021-05-05T09:34:20Z

@christophercr It is now part of GitHub Actions:

stark/.github/workflows/build.yml

Lines 165 to 174 in fb6e448

    
           - name: Combine coveralls reports 
        
             run: node combine-packages-coverage.js 
        
             if: env.IS_MAIN_ENVIRONMENT == 1 
        
           - name: Coveralls 
        
             uses: coverallsapp/github-action@master 
        
             with: 
        
               github-token: ${{ secrets.GITHUB_TOKEN }} 
        
               path-to-lcov: "combined-lcov.info" 
        
             if: env.IS_MAIN_ENVIRONMENT == 1

EDIT: I didn't notice the check Coveralls is not present here. I'm having a look 👍

christophercr · 2021-05-05T09:41:24Z

Ok, but what about the comment posted by Coveralls in the PRs to know whats the new coverage?

christophercr · 2021-05-05T09:42:57Z

Moreover, you can see in the Coveralls page (https://coveralls.io/github/NationalBankBelgium/stark) that the latest merge you did some minutes ago about the NGRX actions is not shown in that page 🤔

SuperITMan · 2021-05-05T10:03:49Z

It's there again

https://coveralls.io/builds/39371204 😊

christophercr · 2021-05-05T10:34:15Z

Alright!! Much better! 😋

coveralls · 2021-05-05T11:53:33Z

Coverage increased (+2.9%) to 88.157% when pulling 4e83f53 on SuperITMan:feature/replace-karma-typescript-by-karma into fb6e448 on NationalBankBelgium:master.

nicanac

Seems really great, I just have a concern regarding the security issue -> snyk.
I'm not well informed about this security test, because when I'm not able to get the details and the reason why it fails.

Otherwise, testing are working well on my win10 pc and on my mac Os.

SuperITMan · 2021-05-06T14:59:36Z

@nicanac
Actually, Snyk is complaining because karma v5 and @angular-devkit/build-angular v0.803.29 contain some vulnerabilities.
Those vulnerabilities should be solved once we migrate on Angular 9 and Karma 6 😊

SuperITMan added comp: testing breaking change type: feature comp: stark-testing labels Apr 29, 2021

SuperITMan added this to the 11.0.0-beta.0 milestone Apr 29, 2021

SuperITMan force-pushed the feature/replace-karma-typescript-by-karma branch 6 times, most recently from c60bd13 to d1fe5a1 Compare April 30, 2021 12:32

SuperITMan marked this pull request as ready for review April 30, 2021 12:50

SuperITMan changed the title ~~Feature/replace karma typescript by karma~~ Feature/replace karma-typescript by karma May 3, 2021

christophercr reviewed May 3, 2021

View reviewed changes

SuperITMan force-pushed the feature/replace-karma-typescript-by-karma branch 3 times, most recently from 2e600fc to 543e498 Compare May 3, 2021 15:40

SuperITMan requested a review from nicanac May 5, 2021 09:24

SuperITMan added 6 commits May 5, 2021 11:25

build(stark-testing): move install location of stark-testing dependen…

c13709f

…cies in root folder Stark-Testing dependencies are now installed in Stark root folder instead of being installed in stark-testing folder

test(showcase): update test configuration after stark-testing changes

6fbedff

test(starter): update test configuration after stark-testing changes

5986250

+ Adapt GitHub Actions script to run starter tests

ci(build-main): upgrade npm from 6.9.2 to latest-6

b986b39

ci(build-main): remove obsolete replacement in combine-packages-cover…

4e83f53

…age script

SuperITMan force-pushed the feature/replace-karma-typescript-by-karma branch from 543e498 to 4e83f53 Compare May 5, 2021 09:28

christophercr approved these changes May 5, 2021

View reviewed changes

nicanac reviewed May 6, 2021

View reviewed changes

SuperITMan merged commit 4c6c9a6 into NationalBankBelgium:master May 6, 2021

SuperITMan deleted the feature/replace-karma-typescript-by-karma branch May 6, 2021 15:00

This was referenced May 7, 2021

testing: replace karma-typescript by karma + @angular-devkit/build-angular:karma #2653

Closed

starter: breaks NodeJS 12 - npm 6.9.2 during install #2495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/replace karma-typescript by karma #2659

Feature/replace karma-typescript by karma #2659

SuperITMan commented Apr 29, 2021

SuperITMan commented Apr 30, 2021

christophercr left a comment •

edited

Loading

christophercr May 3, 2021 •

edited

Loading

SuperITMan May 3, 2021

christophercr May 3, 2021

christophercr May 3, 2021

christophercr May 3, 2021

SuperITMan May 3, 2021

christophercr May 3, 2021

christophercr May 3, 2021

SuperITMan May 3, 2021

christophercr May 3, 2021

SuperITMan May 3, 2021

SuperITMan commented May 3, 2021

sonarqubecloud bot commented May 5, 2021

christophercr commented May 5, 2021

christophercr left a comment

SuperITMan commented May 5, 2021 •

edited

Loading

christophercr commented May 5, 2021

christophercr commented May 5, 2021

SuperITMan commented May 5, 2021

christophercr commented May 5, 2021

coveralls commented May 5, 2021

nicanac left a comment

SuperITMan commented May 6, 2021

		import "zone.js/dist/zone";
		import "zone.js/dist/zone-testing";

Feature/replace karma-typescript by karma #2659

Feature/replace karma-typescript by karma #2659

Conversation

SuperITMan commented Apr 29, 2021

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

SuperITMan commented Apr 30, 2021

christophercr left a comment • edited Loading

Choose a reason for hiding this comment

christophercr May 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperITMan commented May 3, 2021

sonarqubecloud bot commented May 5, 2021

christophercr commented May 5, 2021

christophercr left a comment

Choose a reason for hiding this comment

SuperITMan commented May 5, 2021 • edited Loading

christophercr commented May 5, 2021

christophercr commented May 5, 2021

SuperITMan commented May 5, 2021

christophercr commented May 5, 2021

coveralls commented May 5, 2021

nicanac left a comment

Choose a reason for hiding this comment

SuperITMan commented May 6, 2021

christophercr left a comment •

edited

Loading

christophercr May 3, 2021 •

edited

Loading

SuperITMan commented May 5, 2021 •

edited

Loading