Replace Spectator + Jest with Jasmine and default Testbed #696

netzartist · 2021-08-03T17:32:11Z

Resolves #539

This is "just" to have a proper foundation again for unit tests (after Spectator and suprisingly Jest as of latest Angular 12.x didn't work as nicely as expected). Fixing existing or adding more tests (preferably with Cypress to gain better E2E test confidence) will be tackled in other issues.

stitakis · 2021-08-30T14:44:17Z

client/src/app/modules/project-page/components/project-page.component.spec.ts

@@ -60,9 +85,9 @@ describe('ProjectPageComponent', () => {
  xit('should show section to add new quickstarters when edit-button is clicked', () => {
    /* given */
    /* when */
-    spectator.click('[data-test-edit-btn]');
-    spectator.detectComponentChanges();
+    // spectator.click('[data-test-edit-btn]');


these lines are commented our, they can be removed

even better: the test is commented out (xit()) 😉

netzartist · 2021-08-31T10:29:40Z

@cschweikert do you have time to review this, too?

cschweikert

Hi, sry for the long response time 😅 I reviewed only half of it so far, but wanted to bring already some comments to the table.

cschweikert · 2021-09-06T14:37:56Z

client/karma.conf.js

+    colors: true,
+    logLevel: config.LOG_INFO,
+    autoWatch: true,
+    browsers: ['Chrome'],


I'd recommend to use headless Chrome for also beeing able to run this within the build pipelines. The following configuration does the trick:

... browsers: ['ChromeNoSandboxHeadless'], customLaunchers: { ChromeNoSandboxHeadless: { base: 'Chrome', flags: [ '--no-sandbox', // See https://chromium.googlesource.com/chromium/src/+/lkgr/headless/README.md '--headless', '--disable-gpu', // Without a remote debugging port, Google Chrome exits immediately. '--remote-debugging-port=9222', '--js-flags="--max_old_space_size=2048"', ], }, }, ...

This is also what the angular quickstarter usually uses.

cschweikert · 2021-09-06T14:47:46Z

client/package.json

-    "test": "jest",
-    "test:watch": "jest --watch",
+    "test": "ng test",
+    "test:watch": "ng test --watch",


If I'm not mistaken ng test is already watching by default. I'd propose to provide something like this:

"test": "ng test --watch=false --progress=false --code-coverage", "test:watch": "ng test",

BTW: "test" is similar to the one in the quickstarter (https://github.com/opendevstack/ods-quickstarters/blob/8a236d71902c8fb203fc944bf9d93ee34d16ab03/fe-angular/files/package.json#L8).

cschweikert · 2021-09-06T14:53:11Z

CHANGELOG.md

@@ -4,6 +4,7 @@

 ### Added

+- SPA: Replace Spectator unit test framework by Angular Testbed default ([#539](https://github.com/opendevstack/ods-provisioning-app/issues/539))


Isn't it replacing Jest with Jasmine/Karma (Angular default)? Ah, I guess Jest is using @ngneat/spectator somewhere, right? I would at least mention Jest in the changelog. Also I would mention the Angular update from 12.1.2 to 12.1.4.

cschweikert · 2021-09-06T14:53:55Z

client/package.json

+    "@angular-eslint/template-parser": "^12.3.1",
+    "@angular/cli": "~12.1.4",
+    "@angular/compiler-cli": "~12.1.4",
+    "@angular/language-service": "~12.1.4",
    "@ngneat/spectator": "^8.0.1",


Is @ngneat/spectator still needed in the devDependencies?

cschweikert · 2021-09-06T19:46:11Z

client/src/app/modules/project-page/components/header.component.spec.ts

@@ -37,7 +27,7 @@ describe('ProjectHeaderComponent', () => {
        imports: [
          ProjectModule,
          LoadingIndicatorModule,
-          MatIconModule,
+          MatIconTestingModule,


When I remember it correctly you would need both MatIconModule and MatIconTestingModule for mocking the icons.

cschweikert · 2021-09-06T19:48:43Z

client/src/app/modules/project-page/components/quickstarter-list.component.spec.ts

-        imports: [CommonModule, HttpClientTestingModule, MatIconModule, MatTooltipModule, MatExpansionModule, MatDialogModule],
-        declarations: [QuickstarterListComponent],
+        imports: [CommonModule, HttpClientTestingModule, MatIconTestingModule, MatTooltipModule, MatExpansionModule, MatDialogModule],
+        declarations: [QuickstarterListComponent, MatIcon],


out of curiosity: Why is MatIcon needed here?

cschweikert · 2021-09-07T12:12:28Z

client/src/app/modules/project-page/components/project-page.component.spec.ts

@@ -60,9 +85,9 @@ describe('ProjectPageComponent', () => {
  xit('should show section to add new quickstarters when edit-button is clicked', () => {
    /* given */
    /* when */
-    spectator.click('[data-test-edit-btn]');
-    spectator.detectComponentChanges();
+    // spectator.click('[data-test-edit-btn]');


even better: the test is commented out (xit()) 😉

cschweikert · 2021-09-07T12:17:38Z

client/src/app/modules/project-page/components/project-page.component.spec.ts

+        NotificationModule
+      ],
+      providers: [
+        { provide: API_PROJECT_DETAIL_URL, useValue: '/api/mock' },


I saw this block with API_... in a couple of spec files. Maybe it makes sense to put it somewhere more central. Would it work to have something like a common test module e.g. with all the Mat... modules and this block in the providers section? With this it would be sufficient to only add this new testing module in every spec file.

cschweikert · 2021-09-07T12:24:08Z

client/src/app/modules/new-project/components/new-project.component.spec.ts

-            useValue: {
-              saveItem: jest.fn()
-            }
+            useValue: mockStorageService


mockStorageService is used only here. Maybe simply useValue: {saveItem: () => {}} or useValue: jasmine.createSpy('saveItem')?

stitakis · 2021-11-12T10:36:36Z

@netzartist @cschweikert what is the status of this PR? Can we get it done soon or should we cancel it?

netzartist · 2021-11-12T10:50:42Z

@stitakis Once I find enough blocked time to react to the proposed useful code changes ... Beginning of December I'd estimate. Cancelling is no option.

netzartist · 2022-02-04T13:53:49Z

Agree to close this due to these overall reasons to go into standby with the development of a new frontend for the prov app:

it's unclear how provisioning should work in the era of ODS pipelines
there are other ideas which kind of UI might be more useful to implement
no use case to qualify for a strong focus (and a allocation of dev time) on implementing an UI

Also, the implemented Angular stack is outdated, there are better / leaner ways nowadays.

Replace Spectator + Jest with Jasmine and default Testbed

364bfc7

netzartist added the frontend-spa Issues belonging to SPA Angular frontend label Aug 3, 2021

netzartist requested a review from cschweikert August 3, 2021 17:32

netzartist self-assigned this Aug 3, 2021

netzartist requested a review from stitakis as a code owner August 3, 2021 17:32

stitakis reviewed Aug 30, 2021

View reviewed changes

cschweikert requested changes Sep 6, 2021

View reviewed changes

cschweikert requested changes Sep 7, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Spectator + Jest with Jasmine and default Testbed #696

Replace Spectator + Jest with Jasmine and default Testbed #696

netzartist commented Aug 3, 2021

stitakis Aug 30, 2021

cschweikert Sep 7, 2021

netzartist commented Aug 31, 2021

cschweikert left a comment

cschweikert Sep 6, 2021

cschweikert Sep 6, 2021

cschweikert Sep 6, 2021

cschweikert Sep 6, 2021

cschweikert Sep 6, 2021

cschweikert Sep 6, 2021

cschweikert Sep 7, 2021

cschweikert Sep 7, 2021

cschweikert Sep 7, 2021

stitakis commented Nov 12, 2021

netzartist commented Nov 12, 2021 •

edited

Loading

netzartist commented Feb 4, 2022

		@@ -4,6 +4,7 @@

		### Added

		- SPA: Replace Spectator unit test framework by Angular Testbed default ([#539](https://github.com/opendevstack/ods-provisioning-app/issues/539))

Replace Spectator + Jest with Jasmine and default Testbed #696

Are you sure you want to change the base?

Replace Spectator + Jest with Jasmine and default Testbed #696

Conversation

netzartist commented Aug 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netzartist commented Aug 31, 2021

cschweikert left a 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

stitakis commented Nov 12, 2021

netzartist commented Nov 12, 2021 • edited Loading

netzartist commented Feb 4, 2022

netzartist commented Nov 12, 2021 •

edited

Loading