count header should say total number of experiments fixes #567 #969
Conversation
| reverse("home"), **{settings.OPENIDC_EMAIL_HEADER: user_email} | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| html = response.content.decode("utf-8") |
There was a problem hiding this comment.
From skimming the rest of the file, all other tests, that use self.client don't test the final output but instead test the content (aka. context) of the variables sent into the HTML rendering. My test is, as far as I can see, the first to actually study the generated HTML. The reason for this strong. Had I just tested the response.context I would not have been able to test that the Django template rendering did the right thing. All I'd find is the paginator object.
Also, in this test I convert the response content to a string and use regular expressions. It works but regular expressions never feel great since they can be fragile.
I think my test, as it is now, is very much a scripted way of what a human would do by rendering the HTML in a browser and looking at the output.
There was a problem hiding this comment.
I think this is a good idea.
In theory, we could test both that the view generates the right context, and separately that the template renders the right thing given some context. That would treat the view and the template as loosely coupled components, whereas testing them like this treats the view and component as a tightly coupled unit. I think that better reflects how we often use Django templates.
I would like to see something more robust than regex to validate the output, but this seems fine for now. In the past I would have recommended pyquery, but it seems like it has been abandoned? I'm not sure what a good tool would be now.
There was a problem hiding this comment.
I wouldn't mind using PyQuery! That's what we did on crashstats in the socorro project to investigate what rendered in the Jinja templates.
Mind you, had this been PyQuery it would still have to look something like this:
doc = PyQuery(response.content.decode("utf-8"))
for header in doc('h2.title').items():
assert re.findall(r'\d+\s+Experiments', header.text())Since the string "21 Experiments" isn't in the generated HTML.
There was a problem hiding this comment.
@peterbe @mythmon In all the time that I've been a Django developer I have generally steered clear of validating HTML in unit tests becuase it can change so frequently and I don't think that Django supports very well developed patterns of cleanly testing it. In React it's much more idiomatic to test and validate the output of individual components, that's one of the reasons I like it, and maybe someday we'll migrate the frontend of Experimenter to React and can test the UI more rigourously. In the meantime, I think it's fine that you've added this test here but I don't think it's worth expanding this practice to the project in general, I think just checking in the browser, including screenshots in PRs, and letting QA pass over things is sufficient.
There was a problem hiding this comment.
A couple of points...
I think I made a mistake when using response.content.decode("utf-8") to get to the HTML as a Unicode string. The proper way is content = response.content; content = content.decode(response.charset). Because that's how SimpleTestCase._assert_contains does it.
The QA that my test added would not have been possible because the critical bug was in the template. Not in the view. E.g.
def myview(request):
return render(request, 'template.html', foo="bar")<!-- template.html -->
<body>{{ fooo }}</body>Had you done self.assertEqual(response.context['foo'], 'bar') you might have missed the typo in the the template.
Yes, testing the output is fragile but testing implementation details are even worse :)
|
|
||
| {% if filter.form.owner.value %} | ||
| by {{ filter.form.get_owner_display_value }} | ||
| {% endif %} | ||
|
|
||
| {% if is_paginated and page_obj.number > 1 %} | ||
| <small class="text-muted">Page {{ page_obj.number }}</small> |
There was a problem hiding this comment.
I took the liberty to add this. I know it's a bit out of context for the issue but it certainly feels and looks great. I'm happy to change it to whatever you prefer but I think it's good UI practice to indicate, in some form, that what you're looking at is page X.
Also, omitted to say "Page 1" if you're on page 1. That's just to reduce the noise. Unless someone is clicking around on the paginator (at the bottom) they most "oftenly" don't care that you're on page 1.
There was a problem hiding this comment.
I like this, especially because with the default page size (50, I think?) the paginator is very far away from the top. I think it would be ever better as a "Page 1 of X" indicator, but I'm not sure about that.
There was a problem hiding this comment.
I'm not strongly opinionated on the best practice of that.
At the risk of over-analyzing this; this is what Google does with page 1 and page 2:
There was a problem hiding this comment.
I just noticed this, it does look good!
| reverse("home"), **{settings.OPENIDC_EMAIL_HEADER: user_email} | ||
| ) | ||
| self.assertEqual(response.status_code, 200) | ||
| html = response.content.decode("utf-8") |
There was a problem hiding this comment.
I think this is a good idea.
In theory, we could test both that the view generates the right context, and separately that the template renders the right thing given some context. That would treat the view and the template as loosely coupled components, whereas testing them like this treats the view and component as a tightly coupled unit. I think that better reflects how we often use Django templates.
I would like to see something more robust than regex to validate the output, but this seems fine for now. In the past I would have recommended pyquery, but it seems like it has been abandoned? I'm not sure what a good tool would be now.
|
|
||
| {% if filter.form.owner.value %} | ||
| by {{ filter.form.get_owner_display_value }} | ||
| {% endif %} | ||
|
|
||
| {% if is_paginated and page_obj.number > 1 %} | ||
| <small class="text-muted">Page {{ page_obj.number }}</small> |
There was a problem hiding this comment.
I like this, especially because with the default page size (50, I think?) the paginator is very far away from the top. I think it would be ever better as a "Page 1 of X" indicator, but I'm not sure about that.
|
Since 962 was closed as a dupe, the commit's title will need to be changed to point to #567 instead. |
You mean, a separate issue/PR that is unrelated (but close in topic proximity)? I'm indifferent. There's plenty of other work to do so if Jared is OK with the unrelated change I'm happy to make a cheeky shortcut here to avoid the extra paperwork of a separate issue. |
|
I'm also indifferent. I think that this work is related enough, since it makes the UI better communicate what's going on. Either way, this still needs an updated commit message and PR title before merging. |
I did that. https://github.com/mozilla/experimenter/pull/969/commits If you "Rebase and merge" will it take the GitHub PR title as the commit message or is that only if you do "Squash and rebase"? |
mythmon
changed the title
peterbe
deleted the
962-count-header-should-say-number-of-experiments-across-all-pages
branch
Fixes #962
It's a rather small change but I would appreciate some feedback on the added "Page X" tag and how I did the tests. I'll make some comments within the PR.