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.

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(review): fix tests for review router #683

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

seaerchin
Copy link
Contributor

Problem

Last release added a bunch of checks for sites that are undergoing a migration from github into our db (but not yet completed); this clashes with our code, which expects a fully migrated site (sites table in db populated, site members retrieved from db etc).

This leads to a couple of tests failing in review.spec.ts as the calls to calculate those pre-conditions are not mocked. This PR fixes this by mocking said calls.

Solution

for methods that have the new checks, there is a new beforeEach hook added to mock the call to the precondition. This causes the check to be false (ie, that the site has underwent a full migration), and continues on with the business logic rather than short-circuiting

@seaerchin seaerchin requested a review from a team April 6, 2023 05:09
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

I think the cleaner way of doing this would be to also insert the list of collaborators by adding user/site db entries before each of these tests. I'm fine with releasing this as is for now, but we should change it later on

@seaerchin
Copy link
Contributor Author

I think the cleaner way of doing this would be to also insert the list of collaborators by adding user/site db entries before each of these tests. I'm fine with releasing this as is for now, but we should change it later on

this isn't an integration test so the service itself is mocked and there's no actual db calls being made. the integration test itself is fine as those actually exist

@seaerchin seaerchin merged commit 898ea98 into develop Apr 6, 2023
@seaerchin seaerchin deleted the fix/review-router-tests branch April 6, 2023 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants