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

Initial API testing lesson. #25401

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

linkonsat
Copy link
Contributor

Because

This lesson adds the initial draft of the API testing lesson.

This PR

This PR adds the API testing lesson to the curriculum. One important thing to note is that the other lessons cover a lot of prerequisite knowledge such as knowing about factory-bot. As such this lesson is a little bit shorter than the other ones.

Issue

Closes #24492

Additional Information

Pull Request Requirements

  • I have thoroughly read and understand The Odin Project Contributing Guide
  • The title of this PR follows the location of change: brief description of change format, e.g. Intro to HTML and CSS lesson: Fix link text
  • The Because section summarizes the reason for this PR
  • The This PR section has a bullet point list describing the changes in this PR
  • If this PR addresses an open issue, it is linked in the Issue section
  • If any lesson files are included in this PR, they have been previewed with the Markdown preview tool to ensure it is formatted correctly
  • If any lesson files are included in this PR, they follow the Layout Style Guide

@github-actions github-actions bot added Content: Ruby on Rails Involves the Ruby on Rails course Status: Needs Triage labels Apr 29, 2023
@KevinMulhern KevinMulhern requested review from a team and removed request for a team April 29, 2023 22:41
@rlmoser99
Copy link
Member

Can you share the reasons behind needing an API testing lesson? I am struggling to see the need for this lesson because in the Rails section we don't have people make their own rails API project.

@linkonsat
Copy link
Contributor Author

I'll have to make a few tweaks to this lesson I just realized. That said I have been thinking it over and that is a fair point of criticism. From my understanding it's important to test requests to ensure they work properly and give the expected results, but I do think that is a pretty fair point that we don't have an API project and it may go against the flow of the course structure. to have a lesson that isn't incorporated into a project. So i'm not against dropping it as this brings up a solid point against it. It could be better suited as part of an API standalone module after the rails content now that i'm thinking about it as well. What's your thoughts on that? @rlmoser99

@rlmoser99
Copy link
Member

@linkonsat I've been thinking about this and wonder if we should ask the rails-path-maintainers about this lesson? I lean towards having it when we add content on creating a rails API, but I'd feel much more confident of this decision if more people weighed in on it.

@linkonsat
Copy link
Contributor Author

Sure, I wouldn't mind asking everyone first.

Copy link
Member

@ChargrilledChook ChargrilledChook left a comment

Choose a reason for hiding this comment

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

@linkonsat I appreicate the work you've done for this, it's good to get a start on testing lessons for Rails after this has been one of our biggest gaps.

I have a few concerns about this lesson as it stands right now. I was going to leave detailed feedback about specific parts, but until we sort out the big picture for this lesson I'll focus my comments there.

Broadly, I agree with @rlmoser99 that this lesson doesn't really make sense with the current course structure. Given we have no testing lessons at all at the moment, testing a very specific type of app that we don't actually cover feels a bit awkward.

Despite that, there isn't really anything specific about testing API's in this lesson. It's more a very short description / example of a request spec. While I think it's fair to say request specs are generally more useful for an API only app, they're really more specific to integration testing / testing controllers than APIs.

That might be a more useful thing to have a lesson on, but it would probably need to pivot a decent amount first. And even if we did want to cover request specs specifically, I would argue it's lower priority than model and system / feature specs.

In addition to those issues, I think the content of this lesson is a bit thin at the moment. Half of the lesson is about FactoryBot (I know you have mentioned that we're assuming this is prior knowledge, but it's hard to judge in a vacuum) and the second half is a single example.

I think it would be more useful to explain useful concepts in this area of testing, just as some rough examples:

  • How request specs are a type of integration testing, and what integration testing is
  • When request / integration testing is valuable
  • Examples of different kinds of requests and scenarios you might test
    • ie testing valid vs invalid parameters, being authenticated vs not
  • Thinking of request testing as a black box, and how we're looking for particular http responses for a particular set of inputs

As it is now, I don't think the example is correct either. We're persisting an object to the db before the test, then making a POST request to create the same object (I think? It's a bit unclear). Finally the expectation is testing the object against itself. Unless I'm misunderstanding, we could delete the before block and the test would still pass.

More generally I think we shouldn't be putting the thing we're testing in a before block, and our final expectation should be about http status codes / redirects - maybe JSON responses if we're test an API or a controller that has a JSON response.

See the Rspec Rails docs or even Odin's specs for some examples of what we're trying to achieve with a request spec.


To summarise, I don't think it makes sense to have an API testing lesson at the moment. It might make sense to have a request testing lesson, but we would need to revise and pivot the overall direction of the lesson a little.

I do appreciate the work you've put it into this, so I apologise if my criticism comes off as harsh. I just want to make sure we're delivering the best content we can for our learners.

As a side note, do we have an issue somewhere that is tracking the overall plan for the testing lessons?

@KevinMulhern
Copy link
Member

Sorry, this is my bad. I remember calling this API testing back when we were brain storming these lessons. My thinking at that time was a lesson that taught request specs with VCR to test third party library API's like we have with the Flicker project.

I agree with Chook, a more general request specs lesson would better slot into the current course. Although theres a very good argument as to wether we should skip teaching request spec in the absence of traditional Rails API lessons.

@ChargrilledChook
Copy link
Member

@KevinMulhern I'm partial to that argument and think the bulk of our content should be model and system feature specs.

That said I think Jason's content can often be very opinionated. It's not that I necessarily disagree in this particular case, more about trying to keep tone neutral if that makes sense

@linkonsat
Copy link
Contributor Author

@KevinMulhern I think that would be a bit of a better fit for the standalone section. I was pondering it for a bit and maybe testing third party api libraries would be good for before the flicker project itself. That said it sounds like the conversation is leaning more towards having this as covering the generic area of testing requests, integration, the different responses etc.

@ChargrilledChook no worries. I think this discussion helps with setting the tone of what needs to be changed. All critiques are welcome. If i'm understanding right overall, it's a bit to narrow and needs to cover more stuff in the generic area of testing requests and so on.

@linkonsat
Copy link
Contributor Author

Sorry, this took a bit longer than expected. That said I ultimately did decide it would be better to just have it be an integration test. As it seems like that would be the next type of test after unit/model. Took in your input @ChargrilledChook to cover those other things you mentioned. I did wrap in requests more so by as having it as something you can look for in your tests as well. Interested in what yall think!

Copy link
Contributor

@kamranzafar4343 kamranzafar4343 left a comment

Choose a reason for hiding this comment

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

Just reviewing it, Thank you all for your contribution! I appreciate the effort you've put into this commit. The code looks clean and well-organized. Keep up the good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content: Ruby on Rails Involves the Ruby on Rails course
Projects
No open projects
Status: 🆕 Needs Review
Development

Successfully merging this pull request may close these issues.

Add api testing lesson.
6 participants