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

Improve usage checks in Request and Websocket #162

Closed
gavv opened this issue Nov 23, 2022 · 6 comments · Fixed by #230
Closed

Improve usage checks in Request and Websocket #162

gavv opened this issue Nov 23, 2022 · 6 comments · Fixed by #230
Assignees
Labels
feature New feature or request help wanted Contributions are welcome important Important task
Milestone

Comments

@gavv
Copy link
Owner

gavv commented Nov 23, 2022

Request and Websocket have the following workflow:

  • create instance
  • likely, call WithXXX() methods multiple times
  • call Expect() method

It is not allowed to call WithXXX() after Expect() is called. It is also not allowed to call Expect() more than once. However, these requirements are not documented and checked.

What we should do:

  • Document these requirements in comment for Expect.
  • Report failure if these requirements are violated with Type set to AssertUsage and an explanatory message.
  • Add unit tests that cover these failure reports.
@gavv gavv added feature New feature or request help wanted Contributions are welcome labels Nov 23, 2022
@gavv gavv changed the title Add more usage checks Improve usage checks in Request and Websocket Nov 23, 2022
@gavv gavv added the important Important task label Dec 29, 2022
@chunweii
Copy link
Contributor

Hi @gavv, may I work on this?

I am not sure how to check for the violations. The WithXXX() and Expect() functions are only available for both Request and Websocket structs. The Expect() function returns a *Response or a *WebsocketMessage which do not have the Expect() function. If the user of this library makes this mistake, then the code would fail to build. Doesn't this already check for the violations?

@gavv
Copy link
Owner Author

gavv commented Jan 18, 2023

Hi,

I mean the follwoing misuses:

req := e.GET("/test")

req.WithXXX(...) // ok
req.WithXXX(...) // ok
req.Expect() // ok

req.WithXXX() // bad, With not allowed after Expect; should report failure
req.Expect() // bad, multiple Expect calls not allowed; should report failure

@gavv
Copy link
Owner Author

gavv commented Jan 18, 2023

Let me know if you would like to be assigned.

@chunweii
Copy link
Contributor

Yes please, thanks!

@chunweii
Copy link
Contributor

For Websocket, some of the tests in e2e calls Expect() multiple times. If the usage checks are implemented, these tests will fail.

@gavv
Copy link
Owner Author

gavv commented Jan 23, 2023

Oh, you're right. Websocket.Expect is intended to be called multiple times, once for each message. Seems it's also okay to call WithXxx methods between Expect calls.

So no modifications are needed in Websocket, this issue is relevant only for Request.

@gavv gavv closed this as completed in #230 Jan 24, 2023
@gavv gavv added this to the v2 milestone Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request help wanted Contributions are welcome important Important task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants