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

Jump to bottom

[#105] Web API integration tests #116

Merged
merged 2 commits into from Jun 16, 2022
Merged

[#105] Web API integration tests #116

merged 2 commits into from Jun 16, 2022

Conversation

DK318
Copy link
Member

@DK318 DK318 commented May 24, 2022 •
edited

Description

Problem

We have only 2 integration tests for Web API.
We should create more tests

Solution

Add basic integration tests for every Web API endpoint.

Related issue(s)

Fixed #

✅ Checklist for your Pull Request

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.

  • Documentation

    • I checked whether I should update the docs and did so if necessary:

Stylistic guide (mandatory)

@DK318 DK318 requested review from dcastro and Kariel-Myrr May 24, 2022 16:17
Kariel-Myrr
Kariel-Myrr reviewed May 24, 2022
View reviewed changes
tests/server-integration/Utils.hs Outdated
Comment on lines 120 to 112
( mconcat
[ header "token" "root"
, "path" =: path
, "field" =: name
, port 8081
]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically we have this repeated all over tests. Mb it would be suitable to extract to Utils.
And it will significantly reduce time to change port/header if needed

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I think we should have a function that captures whatever's common to all (or at least to most) requests.

They all have the same base URL, they all use runReq defaultHttpConfig + req, they all have the token header and the same port.

Then I only have to choose the verb, the rest of the URL, any additional headers, the request body, and ignoreResponse vs jsonResponse.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

tests/server-integration/CopyAndRename/RenameTest.hs Show resolved Hide resolved
@DK318 DK318 force-pushed the dk318/#105-web-api-integration-tests branch 2 times, most recently from 6b207c6 to 4895df1 Compare May 27, 2022 19:28
@DK318 DK318 requested review from dcastro and Kariel-Myrr May 28, 2022 13:31
@dcastro
Copy link
Member

dcastro commented May 30, 2022

Could you please also add a commit deleting the javascript tests?

dcastro
dcastro requested changes Jun 1, 2022
View reviewed changes
tests/server-integration/Utils.hs Outdated Show resolved Hide resolved
tests/server-integration/Utils.hs Outdated Show resolved Hide resolved
tests/server-integration/Utils.hs Outdated Show resolved Hide resolved
tests/server-integration/Create/CreateTest.hs Outdated Show resolved Hide resolved
tests/server-integration/Delete/DeleteTest.hs Show resolved Hide resolved
@DK318 DK318 force-pushed the dk318/#105-web-api-integration-tests branch from bc320a4 to 96dd437 Compare June 5, 2022 16:09
dcastro
dcastro approved these changes Jun 5, 2022
View reviewed changes
tests/server-integration/SetField/SetFieldTest.hs Outdated
$ t1 <= newModifiedDate && newModifiedDate <= t2

assertBool "Dates are equal"
$ responseBody response ^?! key "dateModified" . _JSON == newModifiedDate
Copy link
Member

Choose a reason for hiding this comment

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

This check seems redundant.

let newModifiedDate = responseBody response ^?! key "dateModified" . _JSON @_ @UTCTime
responseBody response ^?! key "dateModified" . _JSON == newModifiedDate

Copy link
Member Author

Choose a reason for hiding this comment

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

set-field: updates the entry's and the field's modified date

Hmm, I thought that we want to check all these things.

Copy link
Member

@dcastro dcastro Jun 6, 2022 •
edited

Choose a reason for hiding this comment

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

Yes, but this particular check in line 160 is comparing an expression against itself:

let newModifiedDate = responseBody response ^?! key "dateModified" . _JSON @_ @UTCTime
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

responseBody response ^?! key "dateModified" . _JSON == newModifiedDate
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Copy link
Member

@dcastro dcastro Jun 6, 2022 •
edited

Choose a reason for hiding this comment

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

set-field: updates the entry's and the field's modified date

You're already checking those two requirements in lines 157 and 162:

  assertBool "Date is not recent"
    $ t1 <= newModifiedDate && newModifiedDate <= t2
   ^^^^^^^^^^^^^^^^^ this checks that the entry's date was modified

  assertBool "Dates are not equal"
    $ responseBody response ^?! key "dateModified" . _JSON == newModifiedDate
  assertBool "Dates are not equal"
    $ responseBody response ^?! key "fields" . key "field" . key "dateModified" . _JSON == newModifiedDate
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ this checks that the field's date was modified

Copy link
Member Author

Choose a reason for hiding this comment

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

😄

tests/server-integration/SetField/SetFieldTest.hs Outdated Show resolved Hide resolved
Kariel-Myrr
Kariel-Myrr approved these changes Jun 9, 2022
View reviewed changes
Copy link
Contributor

@Kariel-Myrr Kariel-Myrr left a comment

Choose a reason for hiding this comment

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

looks good 👍
sry for a big delay

tests/server-integration/Utils.hs
)
deleteRecords = void $ try @Req.HttpException do
executeCommand
DELETE ["delete"]
Copy link
Contributor

Choose a reason for hiding this comment

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

stand out from overall code style
look at 104, 107 ...

@DK318
[#105] Web API integration tests
663a003 
Problems: we have only 2 integration tests for Web API.
We should create more tests

Solution: add basic integration tests for every Web API endpoint.
@DK318
[#105] Delete mocha tests.
61aa20c 
Problem: since we have `server integration` tests in `Haskell` we don't
need having `mocha` tests.

Solution: delete `mocha` tests directory.
@DK318 DK318 force-pushed the dk318/#105-web-api-integration-tests branch from 153d446 to 61aa20c Compare June 16, 2022 12:49
@DK318 DK318 merged commit 4decefc into main Jun 16, 2022
@DK318 DK318 deleted the dk318/#105-web-api-integration-tests branch June 16, 2022 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcastro dcastro dcastro approved these changes

@Kariel-Myrr Kariel-Myrr Kariel-Myrr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Write tests for the Web API
3 participants
@DK318 @dcastro @Kariel-Myrr