chore: CXSPA-4841 Fix unit tests and dependencies #17904
Conversation
| // Disable unhandled error logging | ||
| config.onUnhandledError = () => {}; |
There was a problem hiding this comment.
[MEDIUM]
I'm believe that setting this flag in the global context might affect also behavior of other test suites that run after this test suite. I believe changing the global rxjs settings in beforeEach/All and reseting to original value in afterEach/All would be more bullet-proof and not affect other test suites which run in the same shared global JS enviroment.
Anyway, I'm wondering whether we need to set this config at all, or we can adjust our tests to behave better? Did you consider any alternative solutions, that don't require setting this global rxjs config?
There was a problem hiding this comment.
While I agree, that the config change can be moved to beforeEach/afterEach for better safety, I'm wondering what value-added brings adjusting tests to behave better.
I followed the trend from the previous PRs containing RxJS upgrade changes and there are more usages of config.onUnhandledError in our code, I suggest addressing this (if needed) in a separate ticket.
There was a problem hiding this comment.
Value:
Consistency of tests results. Test suites should not interfere with each other implicitly. E.g. if I run some other test suite in isolation (locally, using --include flag, for debugging purposes) and that suite implicitly dependend on the fact that config.onUnhandledError was set globally (but in other test suite), I might get unexpected errors in my run of the individual test suite. But I wouldn't those errors if I run all tests all together.
If it's a common pattern to set globally config.onUnhandledError in the whole epic branch, it sounds reasonable to address it in a separate sub-PR.
There was a problem hiding this comment.
We can also add a comment that such config is used only for already existing tests and we shouldn't use it for new tests.
There was a problem hiding this comment.
To be sure we're on the same page - I agree it's better to move such a config to beforeEach/afterEach or similar to not change the global scope and I see an added value for this. What I'm wondering about is what added value brings to adjusting already existing unit tests to avoid setting such a config. Let's explore this in a separate task https://jira.tools.sap/browse/CXSPA-4870
There was a problem hiding this comment.
Yup. there are 2 levels of changes:
- SHOULD: move the global rxjs config setting (and resetting) to
before/AfterEach. lets do it in a separate PR - COULD: investigate if we can get rid of setting (and resetting) this config at all - maybe by adapting a little bit the tests bodies. We can to it but don't have to. A potential added value might be reducing the complexity of tests by getting rid of the config setting. But the real return of investment depends on how much more/less complex the test bodies might have to become then...
| plugins: [ | ||
| require('karma-parallel'), | ||
| require('karma-jasmine'), |
There was a problem hiding this comment.
[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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here's the separate ticket for this: https://jira.tools.sap/browse/CXSPA-4869
Co-authored-by: Krzysztof Platis <platonn.git@gmail.com>
feature-libs/checkout/base/core/facade/checkout-delivery-address.service.spec.ts
Outdated
Show resolved
Hide resolved
This PR contains dependency updates and fixes for issues in unit tests and issues captured by unit tests caused by upgrading RxJS to version 7.
closes CXSPA-4841