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

Review of simple form chapter #333

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seddonym
Copy link
Collaborator

@seddonym seddonym commented Dec 19, 2024

Good stuff. I like the way you gradually introduce the idea of a form here.

Reviewing this chapter prompted a couple of more general thoughts.

Functional tests

The Selenium-based functional testing approach involves tradeoffs. For me, the lower-level Django test client (or, even better django-webtest) is a sweeter spot for functional testing. I appreciate we use these in test_views, but they're not really unit tests. I'm not sure whether you get to it later in the book, but I think it would be worthwhile discussing the different options so people don't feel like they absolutely have to use Selenium to have decently tested web applications.

And, more generally - we can't test everything, right? So it's about judging the tradeoffs. I think the readers should feel empowered to see that this is just one way but they should keep reflecting on whether it's a way that works for them. My tests for Django look very different to the ones in this book - both the unit and the functional tests.

Keeping the test suite passing

One of the best things I've taken from our recent TDD dojos is the idea of keeping the test suite passing during refactors, even to the point of doing apparently counter-intuitive things such as copy-pasting code. In this chapter we have the test suite failing for quite a while (even to the point where we're making git commits while the suite is failing).

+ in tips at end

Re. the stray +, here's a screenshot of how it appears to me locally. Maybe it's a problem with how I'm building it though.

image

@seddonym seddonym force-pushed the seddonym-review-simple-form branch from 79b6578 to 720a973 Compare December 19, 2024 08:38
@seddonym seddonym marked this pull request as ready for review December 19, 2024 08:56
@seddonym seddonym requested a review from hjwp December 19, 2024 08:56
// But many developers use the Django test client
// (https://docs.djangoproject.com/en/5.1/topics/testing/tools/#the-test-client)
// for this kind of thing, which doesn't have browser validation built in.
// So we could use that if we really wanted to test the server side validation.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Update: I just clocked that we are doing exactly this in test_views.py. Might be worth just calling it out a bit more?

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.

1 participant