Skip to content

cohttp-eio: migrate eio tests to mdx based tests#948

Merged
mseri merged 2 commits intomirage:masterfrom
bikallem:eio-tests-mock
Dec 12, 2022
Merged

cohttp-eio: migrate eio tests to mdx based tests#948
mseri merged 2 commits intomirage:masterfrom
bikallem:eio-tests-mock

Conversation

@bikallem
Copy link
Copy Markdown
Contributor

This PR updates cohttp-eio tests to use mdx based tests primarily using eio.mock package. This allows tests to be repeatable especially with regards to networks and system time facilities.

@bikallem bikallem force-pushed the eio-tests-mock branch 5 times, most recently from d8b93c3 to 61837dd Compare November 14, 2022 14:46
@bikallem bikallem marked this pull request as ready for review November 14, 2022 14:51
@bikallem
Copy link
Copy Markdown
Contributor Author

@mseri The PR is ready for review now. It is mostly tests changes so please advise if the PR needs a Changelog.

@bikallem
Copy link
Copy Markdown
Contributor Author

/cc @mseri I have rebased the PR against the latest master. Ready for merging once ci are all green.

@bikallem
Copy link
Copy Markdown
Contributor Author

bikallem commented Dec 9, 2022

@talex5 @mseri if there isn't anything outstanding or blocking issues could we merge this PR. I have an upcoming PR (date header related) that uses the mdx/mock based test here.

@mseri
Copy link
Copy Markdown
Collaborator

mseri commented Dec 9, 2022

It is still not merged because I need a bit of time to actually review it, even though it is just moving tests around

host
"/")
|> Client.read_fixed
|> ignore);;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why are you ignoring the result? This isn't testing anything except that the client read from the socket!

(this comment applies to the other tests in this file too)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why are you ignoring the result? This isn't testing anything except that the client read from the socket!

The tests in client.md mostly tests the Cohttp_eio.Client response headers and that we can successfully receive response without any exceptions/errors. However, yes it does seem wasteful to ignore the read_fixed results. I have modified the tests to check for valid response body where possible.

@mseri
Copy link
Copy Markdown
Collaborator

mseri commented Dec 12, 2022

Can you rebase on master and for e push? If tests are green I will merge

@mseri mseri merged commit df6e8fc into mirage:master Dec 12, 2022
@bikallem
Copy link
Copy Markdown
Contributor Author

Thanks.

@bikallem bikallem deleted the eio-tests-mock branch December 12, 2022 16:33
bikallem added a commit to bikallem/ocaml-cohttp that referenced this pull request Jan 6, 2023
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