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.

Sign up for GitHub

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

count header should say total number of experiments fixes #567 #969

Merged
merged 1 commit into from
Feb 22, 2019
Merged

count header should say total number of experiments fixes #567 #969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions app/experimenter/experiments/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime
import decimal
import random
import re
from urllib.parse import urlencode

import mock
Expand Down Expand Up @@ -298,6 +299,37 @@ def test_list_view_filters_and_orders_experiments(self):
list(context["experiments"]), list(filtered_ordered_experiments)
)

def test_list_view_total_experiments_count(self):
user_email = "user@example.com"

number_of_experiments = settings.EXPERIMENTS_PAGINATE_BY + 1
for i in range(number_of_experiments):
ExperimentFactory.create_with_status(
random.choice(Experiment.STATUS_CHOICES)[0]
)

response = self.client.get(
reverse("home"), **{settings.OPENIDC_EMAIL_HEADER: user_email}
)
self.assertEqual(response.status_code, 200)
html = response.content.decode("utf-8")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

total_count_regex = re.compile(
rf"{number_of_experiments}\s+Experiments"
)
self.assertTrue(total_count_regex.search(html))

# Go to page 2, and the total shouldn't change.
response = self.client.get(
"{url}?{params}".format(
url=reverse("home"), params=urlencode({"page": 2})
),
**{settings.OPENIDC_EMAIL_HEADER: user_email},
)
self.assertEqual(response.status_code, 200)
html = response.content.decode("utf-8")
self.assertTrue(total_count_regex.search(html))
self.assertTrue('Page 2' in html)


class TestExperimentFormMixin(TestCase):

Expand Down
8 changes: 6 additions & 2 deletions app/experimenter/templates/experiments/list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

{% block header_content %}
<h3 class="m-0">
{{ experiments.count }}
{{ paginator.count }}

{% if filter.form.type.value %}
{{ filter.form.get_type_display_value }}
Expand All @@ -27,11 +27,15 @@ <h3 class="m-0">
{{ filter.form.get_project_display_value }}
{% endif %}

Experiment{{ experiments|pluralize:"s" }}
Experiment{{ paginator.count|pluralize:"s" }}

{% 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>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

screen shot 2019-02-22 at 8 46 13 am

screen shot 2019-02-22 at 8 46 22 am

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed this, it does look good!

{% endif %}
</h3>
{% endblock %}

Expand Down