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

[NRPTI-1045] TESTS** Fix broken tests in API #1121

Merged
merged 14 commits into from
Oct 12, 2023
Merged

Conversation

LocalNewsTV
Copy link
Contributor

@LocalNewsTV LocalNewsTV commented Oct 6, 2023

Pull Request Standards

  • The title of the PR is accurate
  • The title includes the type of change [HOTFIX, FEATURE, etc]
  • The PR title includes the ticket number in format of [NRPTI-###]
  • Documentation is updated to reflect change

Description

This PR includes the following proposed change(s):

  • running npm audit fix to immediately update non-breaking changes to the existing packages in /api
  • introducing nodemon to dev-dependencies so there's hot reloading
  • can now run npm run test-mac to run API tests on apple silicone
  • fixing 54 broken tests from a model change (and other things)
  • some code cleanup to reduce complexity and appease sonar cloud.
  • Commented out one test that would not pass, but manually entering the test did

Copy link
Contributor

@pinkyandthekane pinkyandthekane left a comment

Choose a reason for hiding this comment

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

Great work on tidying up of a lot of this code! I've added some minor comments after looking over everything

api/src/controllers/collection-controller.js Outdated Show resolved Hide resolved
api/src/controllers/collection-controller.js Show resolved Hide resolved
api/test/factories/factory_helper.js Outdated Show resolved Hide resolved
@LocalNewsTV LocalNewsTV changed the title 1045 api tests [NRPTI-1045] TESTS** Fix broken tests in API Oct 11, 2023
Copy link
Contributor

@pinkyandthekane pinkyandthekane left a comment

Choose a reason for hiding this comment

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

Few comments on here, otherwise looking solid

@@ -152,7 +154,7 @@ describe('Map-Info Controller Testing', () => {

request(app)
.delete(endpoint)
.query(qs.stringify({ mapInfoId: testObjectId.toString() }))
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this is removed in this test, but in other tests it is kept in. Is there a reason for the removal?

Copy link
Contributor Author

@LocalNewsTV LocalNewsTV Oct 12, 2023

Choose a reason for hiding this comment

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

@pinkyandthekane
.query() already converts object entries into query strings like qs.stringify is doing. It should be fine to remove them across the board, but was out of scope for this bit of work.

api/test/tests/controllers/search.test.js Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@LocalNewsTV LocalNewsTV merged commit f8a6c92 into master Oct 12, 2023
5 checks passed
@LocalNewsTV LocalNewsTV deleted the 1045-api-tests branch October 12, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants