Opened 7 years ago

Closed 4 years ago

#11797 closed New feature (wontfix)

Test Client Response content form value parsing

Reported by: Rupe Owned by: Rupe
Component: Testing framework Version: master
Severity: Normal Keywords: test client content form post get
Cc: rpbarlow, kmike Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


Added html parsing to the response object created in the test client to extract the initial form field value to facilitate resubmission via post and get.


response = self.client.get('/')
self.failUnlessEqual(response.status_code, 200, 'Failed to retrieve the standard main page.')

# This retrieves the entire set of form values for the form associated with the field (or form) name supplied 
curVals = response.form_data('btnShow')
curVals['pessoaSelect'] = '2'

response ='/', curVals)
self.failUnlessEqual(response.status_code, 200, 'Failed to retrieve the list of Matriz records for the PessoaID 2 (1)')

Attachments (1)

test_client_form_parsing.diff (11.0 KB) - added by Rupe 6 years ago.
updated to the current trunk and improved

Download all attachments as: .zip

Change History (22)

comment:1 Changed 7 years ago by Rupe

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset

comment:2 Changed 7 years ago by Rupe

I'm working the regression tests for this. Should I put the actual test assertions in tests\regressiontests\test_client_regress\ That seems to be where all the rest of the tests have been placed.

comment:3 Changed 7 years ago by russellm

`regressiontests/test_client_regress/' is as reasonable place as any to put tests for this. If it turns out that this feature requires a _lot_ of tests, then it might be worth introducing a tests directory under test_client_regress for organizational purposes, but otherwise just leave it as is and append your tests to those already present.

comment:4 Changed 7 years ago by Rupe

  • Needs documentation unset
  • Needs tests unset

I'm not sure if it was appropriate to put the method documentation in the middle of the attribute list, but other than that I think it's good to go.

comment:5 Changed 7 years ago by Rupe

  • milestone set to 1.2

comment:6 follow-up: Changed 6 years ago by rpbarlow

As per some discussion, it seems to be agreed that we shouldn't try to duplicate the functionality of projects like Twill. This patch is parsing HTML to get the form data. A more desirable solution is to use the request context to get the form. The down side to that approach is that not all form elements are always available in the context.

comment:8 Changed 6 years ago by rpbarlow

  • Cc rpbarlow added

comment:9 Changed 6 years ago by tobias

  • Triage Stage changed from Unreviewed to Design decision needed

comment:10 in reply to: ↑ 6 Changed 6 years ago by Rupe

Replying to rpbarlow:

As per some discussion, it seems to be agreed that we shouldn't try to duplicate the functionality of projects like Twill. This patch is parsing HTML to get the form data. A more desirable solution is to use the request context to get the form. The down side to that approach is that not all form elements are always available in the context.

I understand the concern about overlap though the context side doesn't seem to be really viable. In addition to not always having all of the elements as you point out (say you have a hard coded hidden field), you can also have conditional templates. You would then have to duplicate the template conditions in your testing logic and thus not test the template logic.

comment:11 Changed 6 years ago by Rupe

An other thought ... One of the other main reasons for creating this logic is to cleanup the testing logic. If you open \test\regressiontests\admin_views\ and search for 'data = {', you will find 19 different locations where post data is constructed manually. It would make for much more readable code if we were retrieving a page/form, modifying the from data, and then re-submitting it.

Also, it would make for better real world tests, because otherwise we have no idea if the forms are being created properly (as I mentioned previously).

Are there currently Twill tests for Django?

comment:12 Changed 6 years ago by russellm

  • milestone 1.2 deleted
  • Version SVN deleted

Changed 6 years ago by Rupe

updated to the current trunk and improved

comment:13 Changed 6 years ago by Rupe

  • milestone set to 1.3
  • Owner changed from nobody to Rupe
  • Status changed from new to assigned
  • Version set to SVN

I understand that this is way to late for 1.2 but I thought I would bring it up to date anyway and I had an improvement I wanted to make anyway

comment:14 Changed 6 years ago by kmike

  • Cc kmike added

This is indeed an unnecessary duplication of work that have been done by twill and WebTest.

Twill can be integrated using django-test-utils, tddspry and django-sane-testing, webtest can be integrated using django-webtest.

With django-webtest you don't lose native django test client features like .template, .context, assertTemplateUsed, assertFormError. And it is able to do much more than just filling initial values for forms. So you can just use django-webtest instead of native django test client for your apps and get almost all django's test client benefits plus an easy and powerful API.

On the other hand, django's test suite can certainly benefit from such feature.

But bundling immature self-baked (maybe high-quality though) implementation looks like NIH. I don't think that bundling mature implementation make sense either. So I think the best way is to leave things as-is. It is even documented that something like twill should be used instead of native test client for integrational-style tests.

comment:15 Changed 6 years ago by Rupe

@kmike Why doesn't it make sense to bundle a mature implementation? I think that we could improve the level of testing by reducing the complexity of the test scripts with this or something like django-webtest.

comment:16 Changed 6 years ago by kmike

There is no decent solution that fits everyone.

Mature usually means a bunch of code.

Last twill release is in 2007, twill's unicode support is just bad and nobody is maintaining twill now. Bundling twill means that fixing twill bugs will become a responsibility of core team while there are already > 1000 open tickets (with patches! not to mention ones without patches) in django bug tracker competing for core team's time.

django-webtest is immature integration layer and is in active development itself. It depends on mature WebTest. WebTest is actively developed by Ian Bicking and bundling it in django will slow down this process (or Django will always have outdated WebTest version). WebTest also depends on WebOb so if we want to bundle WebTest then we will have to bundle WebOb (which is an another separately developed package). WebOb is an alternative to django's Request object so if we bundle django-webtest then we suddenly have 2 Request object implementations in django trunk. That's crazy :)

All of this is not a problem if we don't bundle anything. From app developer point of view the install process for all of these packages is easy and straightforward so if anyone want to use nicer API for test he can do it easily. On other hand, bundling twill or WebTest will require massive test refactoring and will lead to maintain burden. I think it is just not practical in current situation.

comment:17 Changed 6 years ago by Rupe

Ok, with that said, I agree the other options are not a good fit for the trunk.

There is still the reason why I created this patch; to clean up the existing test code and hopefully create more robust testing via the simplicity it affords. Do you believe that is not reason enough to include this patch?

comment:18 Changed 6 years ago by kmike

I like your intentions very much because this is really a better way to write tests than django's current method.

But I think that in practice efforts to support this feature can outweighs benefits it provide.

For example, your patch uses HTMLParser. HTMLParser is known for it's bad parsing of normal real-world (but maybe invalid) HTML. This is one of the reasons there are a lot of other parsing libraries (lxml, BeautifulSoup, html5lib, ...). So there will be tickets complaining about test client html parsing. So there will be patches and HTMLParser workarounds - and this can eventually lead to django's own half-baked html parsing library.

The benefit over using django-test-utils or django-webtest is only an easier testing of django itself.

But converting django test suite to use new method also have some downsides: html parsing takes time and test suite will run longer. Existing suite is huge and converting it is a non-trivial work.

I'm not a core developer and they possibly don't share my point of view so take all these as a personal opinion. This is all about balance and balance is a subjective matter.

comment:19 Changed 5 years ago by baumer1122

  • Severity set to Normal
  • Type set to New feature

comment:20 Changed 5 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:21 Changed 4 years ago by aaugustin

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from assigned to closed
  • UI/UX unset

As pointed out by Russell in the mailing list thread linked above, this is a significant increase of the scope of the test client, and it could be considered feature creep. Parsing HTML is not as trivial a task as it seems, even with Python's stdlib; browsers do incredible things to get it right in almost all cases.

We're about to introduce support for live browser testing (for instance with Selenium) in #2879 and I think it's a better path forwards.

It seems to me that the work on this ticket could live outside of trunk as a subclass of the test client, couldn't it? It would be a good candidate for djangosnippets.

Note: See TracTickets for help on using tickets.
Back to Top