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

Return 500 when DB is not connected (fix for issue 570) (prelanding) #583

Merged
merged 56 commits into from
Mar 24, 2022

Conversation

fgalan
Copy link
Member

@fgalan fgalan commented Feb 18, 2022

Issue #570. Continues the work in PR #574

It can be noted that at PR creation time (at commit 45c2186) PR has +132 -14 in changes, the same as the old PR.

CC: @Gauravp-NEC

To be completed (by @mapedraza )

  • Not try to reconnect upon connection fail. Already mentioned at Return 500 when DB is not connected (fix for issue 570) #574 (comment) but not fully solved
  • The it('should disconnect from the database', ...) clause is not really a step to test but a preparation statement. It should be done in the "before" clause associated to the containing describe() block.

fgalan and others added 3 commits March 16, 2022 11:43
@mapedraza
Copy link
Collaborator

End 2 end tests moved to a new different file + deletion of the other tests done in 9f02447 .

@mapedraza
Copy link
Collaborator

Added tests expectations + more descriptive error message in 3df4d63

As an example, the error response is like the following:

{"statusCode":500,"error":"Internat Server Error","message":"DB not connected"}

entityId: 'E',
attrName: 'A'
},
'A' // maybe unneeded in API_OPERATION.DELETE case (see getURL method)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I already checked that, without this line, tests complete successfully

Suggested change
'A' // maybe unneeded in API_OPERATION.DELETE case (see getURL method)

Copy link
Member Author

Choose a reason for hiding this comment

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

The , in L122 should be removed if L123 is removed, shouldn't be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, was a typo on the review

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in 8c49723

test/unit/sthE2E_test.js Outdated Show resolved Hide resolved
test/unit/sthE2E_test.js Outdated Show resolved Hide resolved
mapedraza and others added 2 commits March 22, 2022 16:26
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
@fgalan fgalan changed the title [WIP] Return 500 when DB is not connected (fix for issue 570) (prelanding) Return 500 when DB is not connected (fix for issue 570) (prelanding) Mar 22, 2022
Copy link
Member Author

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

With the last fixes the PR is ready to be merged! :)

LGTM

Copy link
Collaborator

@mapedraza mapedraza left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

4 participants