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

Fix test framework for #688 #689

Open
ono-max opened this issue Jul 8, 2022 · 7 comments
Open

Fix test framework for #688 #689

ono-max opened this issue Jul 8, 2022 · 7 comments

Comments

@ono-max
Copy link
Member

ono-max commented Jul 8, 2022

#688

@ono-max
Copy link
Member Author

ono-max commented Jul 8, 2022

I'll fix it later.

@st0012
Copy link
Member

st0012 commented Jul 8, 2022

@ono-max
Copy link
Member Author

ono-max commented Jul 8, 2022

Oh, I see.

Could you create the PR instead of me?

@st0012
Copy link
Member

st0012 commented Jul 8, 2022

@ono-max Do you agree to drop the detach raw test? I think in this case it's unnecessary because the request test cover the exact same behavior. The rest of requests have also been tested in other parts of request tests.

@ono-max
Copy link
Member Author

ono-max commented Jul 11, 2022

Thank you for asking me. I think that more test cases are better than less test cases, though?

Although I wrote the raw test in #705, I'll close it if you disagree with it.

@st0012
Copy link
Member

st0012 commented Jul 11, 2022

I think that more test cases are better than less test cases, though?

That's true but we also need to consider the maintenance cost. If you look at #705, you can see that the test content may have been outdated for a while. And that makes me think:

  • If we go stricter on matching the req/res, it'll need to be updated more frequently. But until now the missed changes don't seem to cause issues, so perhaps it's fine to keep it this way?
  • If we decide that a rough behavior match like the current one is good enough, why don't we just maintain the request tests?

I of course appreciate your work on #705 so I don't think we should close it. But if the raw tests in the current form are worth maintaining is debatable imo.

@st0012
Copy link
Member

st0012 commented Jul 11, 2022

How about this: if you can list one or more scenarios that a raw test case covers that its request counterpart doesn't, we keep it. In addition to that, we should also document it done in the file so it'll help us maintain it.
If not, we remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants