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

[5.6] Return 201 in case of a Model just created. #21625

Merged

Conversation

mathieutu
Copy link
Contributor

@mathieutu mathieutu commented Oct 11, 2017

@m1guelpf
Copy link
Contributor

@mathielo Tests?

@mathieutu
Copy link
Contributor Author

@m1guelpf If only I could... 😔
As I've seen, the whole file isn't tested, and IMO the purpose of this PR isn't to add a bunch of tests. I can create a new file for the router and just test the modification, but I had trouble with people in some of my PRs because of tests (when more modifications in tests than in feature), so I'm a bit cautious with that.

@ConnorVG
Copy link
Contributor

Added behaviour? Add some tests.

@ntzm
Copy link
Contributor

ntzm commented Oct 12, 2017

had trouble with people in some of my PRs because of tests

Why would people have trouble with you adding tests?

@mathieutu
Copy link
Contributor Author

mathieutu commented Oct 12, 2017

Added behaviour? Add some tests.

Totally agree..!

Why would people have trouble with you adding tests?

As I said, there is a difference between adding some tests in the middle of others, and creating some new tests classes and way to tests features. When it's only one understandable line added to the code it's ok, but when you add more modifications in the tests that in the features, it can "fear" some people. ¯_(ツ)_/¯

But guys I totally agree with all of you, and I'm a TDD guy, so no need to argue about that with me.
Will add a file to test the router, and some tests for this one.

@taylorotwell
Copy link
Member

You can easily write an integration test for it.

@taylorotwell
Copy link
Member

taylorotwell commented Oct 12, 2017

Look at the routing tests in the Tests/Integrations folder.

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.

5 participants