Fix Spec has no expectations errors #2371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added a expect().nothing() to the test who don't expect anything but just ensure that no error is thrown
Also fixed incorrect regex in isLink()
- Wrong css class was used - The check to see if the offset was defined did not exist
Reverted the way you detect when an item is selected by using observables again instead of promises. The reason why it didn't work without the old observable approach was because `VocabularyTreeviewModalComponent` didn't have a `select` `@Output` like `VocabularyTreeviewComponent`
- renamed `showBackButton` to `showBackButton$` because it's an observable - `showBackButton$` only emits `true` when the back button should be shown, it does not emit `false` when it should be hidden, it just did nothing
- The buttons are now not disabled anymore but hidden when you don't have the correct authorization - Wrong css class was used for the edit buttons
- Wrong css class was used - The check to see if the offset was defined did not exist
The fixture.whenStable() was not awaited
- The isWithdrawn was set twice to mockItem1 - The expected observable was not correctly compared
…y exist to check if an error is thrown
The test's searchData & the searchData from the SubscriptionsDataService were not linked
- The store dispatch was not called through so the store was never updated - To initialize the store using provideMockStore I rewrote the test to work through injection
Because of the switch from BehaviourSubject to Subject the tests always failed because of a timout because the previous state is not kept with Subject and should be manually triggered during the tests
…ponent, CommunityPageSubCommunityListComponent no expectation tests Because their child components use a themed component the changes need to be detected twice, this is a side effect from the component being created dynamically
- Because the custom theme is disabled by default I replaced it with the dspace theme who is still enabled by default - Because the child components use a themed component the changes need to be detected twice, this is a side effect from the component being created dynamically
- Removed duplicate request - Added expect().nothing() because httpMock.expectNone will throw an error when a request is made
alexandrevryghem
added
the
testing framework
|
There are still 2 tests failing, in my opinion they can be removed, but I would prefer having someone else's feedback before deleting them:
|
tdonohue
added
the
1 APPROVAL
|
Hi @alexandrevryghem, |
…xpectations_contribute-main # Conflicts: # src/app/shared/browse-by/shared-browse-by.module.ts
Buttons to which you don't have access are now hidden by default instead of disabled
|
Hi @alexandrevryghem, |
…xpectations_contribute-main
|
Hi @alexandrevryghem, |
…xpectations_contribute-main # Conflicts: # src/app/shared/search/search-filters/search-filter/search-hierarchy-filter/search-hierarchy-filter.component.ts
alexandrevryghem
force-pushed
the
fix-specs-without-expectations_contribute-main
branch
from
db710c5 to
109418b
Compare
|
@alexandrevryghem : I finally got around to reading this PR and your comments above about the failing tests. I agree with your logic that it seems reasonable to remove both of the failing tests. So, if you find time to update this to delete the failing tests & get this passing tests...then I'll do my best to move this forward soon as it seems like a very useful change before we start doing major code updates (to Angular 16, etc) |
…xpectations_contribute-main
Added the necessary done() calls back & fixed tests
|
@tdonohue: All tests are fixed now |
tdonohue
approved these changes
Mar 4, 2024
There was a problem hiding this comment.
👍 Thanks @alexandrevryghem for this necessary improvement/fix! I looked over the code and it looks good. Also verified locally that all tests on existing main pass this check.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Currently there are some tests (in total 60) who don't have any expecations, this is often because of Observables who only emit after the test has already finished. In this PR I enabled a rule to always throw
Spec has no expectationsfor tests without expectations, this PR also fixes those tests. Some tests also don't expect anything because they only exist to check if an error is thrown, for those tests I addedexpect.nothing()at the end to make sure they also succeed with this rule (I also always added some inline explanation on the places where I used this). Enabling this rule would mean that small errors like this one would be catched earlier, instead of making you think that everything is working.Instructions for Reviewers
I committed every test I fixed seperatly with a short explanation with the reason of the change in the commit description, I think this will make it easier to review 🤷.
Checklist
yarn lintyarn check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.