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.

Sign up for GitHub

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

chore: CXSPA-4841 Fix unit tests and dependencies #17904

Merged
merged 5 commits into from
Oct 3, 2023
Merged

chore: CXSPA-4841 Fix unit tests and dependencies #17904

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7b42395
chore: CXSPA-4841 Fix unit tests and dependencies
pawelfras Oct 3, 2023
49c1e41
Merge branch 'epic/CXSPA-2897' into feature/CXSPA-4841
pawelfras Oct 3, 2023
893f730
refactor after review
pawelfras Oct 3, 2023
08b4cdb
Update projects/core/src/util/command-query/command.service.ts
pawelfras Oct 3, 2023
b673cad
refactor after review
pawelfras Oct 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions feature-libs/checkout/base/core/facade/checkout-delivery-address.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,28 @@ class MockCheckoutQueryFacade implements Partial<CheckoutQueryFacade> {
);
}

// configure rxjs to not crash node instance with thrown errors
config.onUnhandledError = (err) => console.warn(err);

describe(`CheckoutDeliveryAddressService`, () => {
let service: CheckoutDeliveryAddressService;
let connector: CheckoutDeliveryAddressConnector;
let checkoutQuery: CheckoutQueryFacade;
let eventService: EventService;

// TODO: CXSPA-4870 verify if can be avoided
let originalOnUnhandledError: ((err: any) => void) | null;

beforeAll(() => {
// configure rxjs to not crash node instance with thrown errors
// TODO: CXSPA-4870 verify if can be avoided
originalOnUnhandledError = config.onUnhandledError;
config.onUnhandledError = () => {};
});

afterAll(() => {
// reset rxjs configuration
// TODO: CXSPA-4870 verify if can be avoided
config.onUnhandledError = originalOnUnhandledError;
});

beforeEach(() => {
TestBed.configureTestingModule({
providers: [
Expand Down
18 changes: 16 additions & 2 deletions feature-libs/order/core/facade/reorder-order.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,29 @@ class MockMultiCartFacade implements Partial<MultiCartFacade> {
deleteCart = createSpy();
}

config.onUnhandledError = (error) => console.warn(error);

describe(`ReorderOrderService`, () => {
let service: ReorderOrderService;
let connector: ReorderOrderConnector;
let userIdService: UserIdService;
let activeCartFacade: ActiveCartFacade;
let multiCartFacade: MultiCartFacade;

// TODO: CXSPA-4870 verify if can be avoided
let originalOnUnhandledError: ((err: any) => void) | null;

beforeAll(() => {
// configure rxjs to not crash node instance with thrown errors
// TODO: CXSPA-4870 verify if can be avoided
originalOnUnhandledError = config.onUnhandledError;
config.onUnhandledError = () => {};
});

afterAll(() => {
// reset rxjs configuration
// TODO: CXSPA-4870 verify if can be avoided
config.onUnhandledError = originalOnUnhandledError;
});

beforeEach(() => {
TestBed.configureTestingModule({
providers: [
Expand Down
3 changes: 1 addition & 2 deletions feature-libs/pdf-invoices/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
module.exports = function (config) {
config.set({
basePath: '',
frameworks: ['parallel', 'jasmine', '@angular-devkit/build-angular'],
frameworks: ['jasmine', '@angular-devkit/build-angular'],
plugins: [
require('karma-parallel'),
require('karma-jasmine'),
Copy link
Contributor

Choose a reason for hiding this comment

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

[QUESTION]
Why is removing karma-parallel related to the upgrade of rxjs?

btw. I can understand that for small libraries that have little unit tests, the parallelization of the unit tests execution might not bring much value. pdf-invoices might be one of such libraries. But I'm wondering why you made this change in this rxjs-upgrad PR ?

Note: analogical question to other files with removed karma-parallel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the same question in my mind when I started reviewing already closed PRs that were related to the RxJS upgrade. It wasn't my decision to remove it and I'm following the trend ;) I asked @ercultimate for the reasons behind such a change and it turned out that karma-parallel doesn't work with the new version of jasmine-marbles. Eric sent me a link to the GitHub issue that contains his PR with the fix, but this library seems to be not maintained anymore (maybe it's another good reason to not use it). That said, I heard someone from the team suggested removing the problematic library as it was used in very few test suites.

Copy link
Contributor

@Platonn Platonn Oct 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Then, shall we remove karma-parallel from all the libraries, or just those that use jasmine-marbles?
Just to better understand the consequences: What's the impact of removing karma-parallel from all libraries on the overall time waiting for CI to pass?

Note: as it's a common pattern in the epic branch, it can be addressed in a separate PR, if neeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the separate ticket for this: https://jira.tools.sap/browse/CXSPA-4869

require('karma-coverage'),
require('karma-chrome-launcher'),
Expand Down
2 changes: 1 addition & 1 deletion feature-libs/pdf-invoices/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"@spartacus/schematics": "6.5.0",
"@spartacus/storefront": "6.5.0",
"@spartacus/styles": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"publishConfig": {
"access": "public"
Expand Down
3 changes: 1 addition & 2 deletions feature-libs/requested-delivery-date/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@
module.exports = function (config) {
config.set({
basePath: '',
frameworks: ['parallel', 'jasmine', '@angular-devkit/build-angular'],
frameworks: ['jasmine', '@angular-devkit/build-angular'],
plugins: [
require('karma-parallel'),
require('karma-jasmine'),
require('karma-coverage'),
require('karma-chrome-launcher'),
Expand Down
2 changes: 1 addition & 1 deletion feature-libs/requested-delivery-date/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
"@spartacus/schematics": "6.5.0",
"@spartacus/storefront": "6.5.0",
"@spartacus/styles": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"publishConfig": {
"access": "public"
Expand Down
54 changes: 36 additions & 18 deletions integration-libs/cdc/user-profile/register/cdc-register-component.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
UserRegisterFacade,
UserSignUp,
} from '@spartacus/user/profile/root';
import { Observable, of } from 'rxjs';
import { config, Observable, of, throwError } from 'rxjs';
import { CDCRegisterComponentService } from './cdc-register-component.service';
import createSpy = jasmine.createSpy;

Expand Down Expand Up @@ -106,6 +106,22 @@ describe('CdcRegisterComponentService', () => {
let fb: UntypedFormBuilder;
let anonymousConsentsService: AnonymousConsentsService;

// TODO: CXSPA-4870 verify if can be avoided
let originalOnUnhandledError: ((err: any) => void) | null;

beforeAll(() => {
// configure rxjs to not crash node instance with thrown errors
// TODO: CXSPA-4870 verify if can be avoided
originalOnUnhandledError = config.onUnhandledError;
config.onUnhandledError = () => {};
});

afterAll(() => {
// reset rxjs configuration
// TODO: CXSPA-4870 verify if can be avoided
config.onUnhandledError = originalOnUnhandledError;
});

beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
Expand Down Expand Up @@ -232,27 +248,29 @@ describe('CdcRegisterComponentService', () => {
});

it('should not do anything when CDC registration fails', (done) => {
cdcJsService.registerUserWithoutScreenSet = createSpy().and.returnValue(
throwError(() => 'ERROR')
);
cdcUserRegisterService.generatePreferencesObject =
createSpy().and.returnValue({});

cdcUserRegisterService
.register({ ...userRegisterFormData, password: undefined })
.subscribe({
error: () => {
expect(connector.register).not.toHaveBeenCalled();
expect(
cdcJsService.registerUserWithoutScreenSet
).toHaveBeenCalledWith({
titleCode: 'Mr.',
firstName: 'firstName',
lastName: 'lastName',
uid: 'uid',
preferences: {},
});
done();
},
});
cdcUserRegisterService.register(userRegisterFormData).subscribe({
error: () => {
expect(connector.register).not.toHaveBeenCalled();
expect(
cdcJsService.registerUserWithoutScreenSet
).toHaveBeenCalledWith({
titleCode: 'Mr.',
firstName: 'firstName',
lastName: 'lastName',
uid: 'uid',
password: 'password',
preferences: {},
});
},
});
expect(cdcJsService.didLoad).toHaveBeenCalled();
done();
});

it('should throw error when CDC user token fails', (done) => {
Expand Down
2 changes: 1 addition & 1 deletion integration-libs/segment-refs/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
"@angular/core": "^15.2.9",
"@spartacus/core": "6.5.0",
"@spartacus/schematics": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"publishConfig": {
"access": "public"
Expand Down
21 changes: 20 additions & 1 deletion projects/core/src/util/command-query/command.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
Observable,
PartialObserver,
Subject,
config,
of,
throwError,
} from 'rxjs';
Expand Down Expand Up @@ -34,6 +35,22 @@ describe('CommandService', () => {
let request1: Subject<string>;
let request2: Subject<string>;

// TODO: CXSPA-4870 verify if can be avoided
let originalOnUnhandledError: ((err: any) => void) | null;

beforeAll(() => {
// configure rxjs to not crash node instance with thrown errors
// TODO: CXSPA-4870 verify if can be avoided
originalOnUnhandledError = config.onUnhandledError;
config.onUnhandledError = () => {};
});

afterAll(() => {
// reset rxjs configuration
// TODO: CXSPA-4870 verify if can be avoided
config.onUnhandledError = originalOnUnhandledError;
});

beforeEach(() => {
TestBed.configureTestingModule({});
service = TestBed.inject(CommandService);
Expand Down Expand Up @@ -273,9 +290,11 @@ describe('CommandService', () => {

it('should cancel in-progress requests', () => {
const observer1 = createObserverSpy('observer1');
cancelPrevCommand.execute(request1).subscribe(observer1);
const sub1 = cancelPrevCommand.execute(request1);
// cancelPrevCommand.execute(request1).subscribe(observer1);

cancelPrevCommand.execute(request2).subscribe();
sub1.subscribe(observer1);
request1.next('next');

expect(observer1.next).not.toHaveBeenCalled();
Expand Down
18 changes: 16 additions & 2 deletions projects/core/src/util/command-query/command.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
zip,
} from 'rxjs';
import {
TapObserver,
catchError,
concatMap,
finalize,
Expand Down Expand Up @@ -53,6 +54,19 @@ export class CommandService implements OnDestroy {
const commands$ = new Subject<PARAMS>();
const results$ = new Subject<ReplaySubject<RESULT>>();

// We have to specify selectively the handlers after RxJS version 7.3.0.
// Otherwise, the `unsubscribe` handler would be forwarded implicitly
// to `notifier$` when calling `tap(notifier$)`.
// But we don't want to forward `unsubscribe`, in particular.
// To see more details, please check: https://github.com/ReactiveX/rxjs/pull/6527
const notify = (
notifier$: ReplaySubject<RESULT>
): Partial<TapObserver<RESULT>> => ({
next: (x) => notifier$.next(x),
error: (e) => notifier$.error(e),
complete: () => notifier$.complete(),
});

let process$: Observable<unknown>;

switch (options?.strategy) {
Expand All @@ -61,7 +75,7 @@ export class CommandService implements OnDestroy {
process$ = zip(commands$, results$).pipe(
switchMap(([cmd, notifier$]) =>
defer(() => commandFactory(cmd)).pipe(
tap(notifier$),
tap(notify(notifier$)),
catchError(() => EMPTY),
finalize(() => {
// do not overwrite existing existing ending state
Expand Down Expand Up @@ -95,7 +109,7 @@ export class CommandService implements OnDestroy {
process$ = zip(commands$, results$).pipe(
concatMap(([cmd, notifier$]) =>
defer(() => commandFactory(cmd)).pipe(
tap(notifier$),
tap(notify(notifier$)),
catchError(() => EMPTY)
)
)
Expand Down
6 changes: 3 additions & 3 deletions projects/schematics/src/dependencies.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@
"@spartacus/schematics": "6.5.0",
"@spartacus/storefront": "6.5.0",
"@spartacus/styles": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"@spartacus/pickup-in-store": {
"@angular-devkit/schematics": "^15.2.9",
Expand Down Expand Up @@ -234,7 +234,7 @@
"@spartacus/schematics": "6.5.0",
"@spartacus/storefront": "6.5.0",
"@spartacus/styles": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"@spartacus/smartedit": {
"@angular-devkit/schematics": "^15.2.9",
Expand Down Expand Up @@ -360,7 +360,7 @@
"@angular/core": "^15.2.9",
"@spartacus/core": "6.5.0",
"@spartacus/schematics": "6.5.0",
"rxjs": "^6.6.0"
"rxjs": "^7.8.0"
},
"storefrontapp": {
"@angular/animations": "^15.2.9",
Expand Down