Test refactor

[anselmbradford]

@monfresh ready for review.

I refactored the test custom value tests that were failing in #332.
I also pulled all the expectations out of test session helpers to address #331 and put them as shared tests in the filter spec. I'm not sure if this is the best place for them, but let's open an issue/make another PR for that if that's not the best way to address those.

[monfresh]

I'd rather address the 2 issues in separate commits, especially since you didn't really address #331. From what I can tell, all you did was move the methods that test functionality from session_helpers to filters_spec.

It's my fault for not being more clear. What I meant is that methods like "test_filter_legend" should not exist at all because it makes the specs themselves hard to read, like this:

scenario 'when location filter has no cached values and legend is toggled', :vcr do
  test_filter_legend("location")
end

I have no idea what this is testing or what the expected behavior is.

"test_filter_legend" acts very much like a spec, so it's like having a spec within a spec.

It's also testing a bunch of things at once. To properly test the filter legend, I would create a separate feature file for it and test each component separately. I would break up filter_spec.rb into various smaller feature files that each test one aspect of the filters. For the most part, each test should only have one expectation, and specs are meant to be readable.

If several tests share the same expected behavior, I would explore RSpec methods of reuse like custom matchers and shared examples.

Helper methods for tests are encouraged when they simplify a set of common actions that can be reused across tests. For example, if you have many tests that visit the homepage, then fill in the search field with a keyword, it would make sense to create a helper method like this:

def visit_homepage_and_search_for(keyword)
  visit "/"
  fill_in "keyword", with: keyword
  click_button "search"
end

Then, in your spec, you can do:

scenario "when searching for 'pantry'" do
  visit_homepage_and_search_for("pantry")
  expect(page).to have_content "Samaritan House"
end

Here are some other resources:

http://betterspecs.org/
https://www.codeschool.com/courses/testing-with-rspec

I can take care of #331 if you prefer. In the meantime, please revert the move of the methods from session_helpers. Thanks!

[anselmbradford]

Okay, reverted the commit. Yeah feel free to tackle #311. What I was trying to avoid is having several different tests for the different toggles, which have identical or nearly identical behavior, other than different IDs and textual content. Shared examples seems like the way to go, but I'd have to read into how that's different from having a shared method directly in the spec.

[monfresh]

Actually, can you make one more change, please, for readability of the specs? For both scenarios, use the values directly, so from this:

filter,value = "location","Custom Value"
fill_filter_custom_field(filter,value)
expect(all("##{filter}-options .current-option label").last).to have_content(value)

to this:

fill_filter_custom_field("location", "Custom Value")
expect(all("#location-options .current-option label").last).
  to have_content("Custom Value")

Remember that above all, specs should be readable.

[anselmbradford]

Okay, DRY isn't a consideration here? I'll push this change, but just wondering. It's fine here b/c "Custom Value" isn't apt to change b/c it's textual content input by the user, but "location" ties into an ID in the HTML that could change for semantic reasons, so it seems useful to separate that out as a variable if the test referred to "location" more than once (which in this case it doesn't—so no worries).

[monfresh]

Specs should try to depend as little as possible on things that might change. In cases where you just have to use a CSS id or class, and if it will be used in many specs, that's when you use a helper method like I described above. Instead of trying to come up with a generic method that will apply to a bunch of different CSS classes or IDs, you should create a separate one for each field. So, for the location field, you would have a helper method that only filled out that field, like this for example:

def filter_by_location(value)
  fill "location", with: value
  click_button "Search"
end

Then in your location field spec (remember, one spec per thing you want to test):

scenario "when the search is filtered by location" do
  filter_by_location("Redwood City, CA")
  expect(page).to have_content "Health Insurance TeleCenter"
end

It looks like you've already done something similar.

The point is that you want to test the feature in the app, not the markup. It's the actual filtering that needs to be tested. I haven't looked in detail at every spec in filters_spec.rb, but it seems like those specs are only testing the javascript portion of the form as you're interacting with them, but before you've actually submitted the filter. If that's the case, I would actually get rid of all of those tests. They're not testing anything useful. The main thing we want to test is whether or not the filtering works, so if there are already specs for each individual filter, then having any that don't actually submit would be redundant.

[anselmbradford]

Yeah, that makes sense, but that's where I'm a little confused on why you wouldn't want to pull those IDs out of the test commands and separate them into a variable at the top—b/c those are the elements in the test that could change, or are at least an element that can change. The IDs aren't always input fields that are being filled in, in this case they are <fieldset> elements that are being clicked. Anyway, you can explain it some other time.

I don't think removing tests that test the toggles before form submission would provide full coverage. The JavaScript manipulates the DOM, so testing needs to happen at all points of interaction that the JS is changing the DOM, not just after form submission.

[monfresh]

Interacting with the form is a prerequisite for submitting the form. If you have a spec that tests submitting the form with the "Near Address" filter, then you've already automatically tested the DOM manipulation in that same spec as well.

This spec on its own has no value and is just slowing down the test suite:

fill_filter_custom_field("location", "Custom Value")
  expect(all("#location-options .current-option label").last).
    to have_content("Custom Value")

Instead, it should continue past the first step to submit the form:

fill_filter_custom_field("location", "Custom Value")
click_button "Search" #or however the form is submitted
expect(page).to have_content "Your location could not be determined!"

Now this is testing something useful. It's testing that an appropriate error message is displayed when the user submits the form with an invalid location. It's also testing the DOM manipulation, which obviates the need for your original spec that only tests the DOM manipulation.

The helper method can then be refactored so that it is more readable and also submits the form, perhaps something like:

scenario "when an invalid custom location is submitted" do
  filter_by_custom_location("Custom Value")
  expect(page).to have_content "Your location could not be determined!"
end

Does this make sense?

[anselmbradford]

It still seems like it's missing an interaction that could be broken. The value that's typed into the location input field is the value that's submitted to the server, which gets tested when the form is submitted, as in your example. That is fine, but additionally when closing the filter toggle the entered value in the location input field is transferred to a <label> node above the form input:

<div class="options current-option">
   ...
   <label for="location-option">Custom Value</label>
   ...
   <input class="" id="location-option-input" list="location-list" name="location-option-input" placeholder="address" type="search" value="Custom Value">
   ...

Which is then displayed when the toggle is closed:

This is what...

expect(all("#location-options .current-option label").last).
    to have_content("Custom Value")

...is checking. If that is broken and what the user entered is not being copied properly to the label node, what is displayed might read as "All", "undefined", an empty string, etc. but the form would still submit fine.

[monfresh]

Perhaps this is indicative of a functionality that's not properly implemented, or that can be simplified?
Instead of copying the user input to the label node, would it be possible to just have the label read the user input so that you know they're always in sync? The value that I see displayed after collapsing the field is what I would expect to be submitted.

In terms of simplification, why not just have an immediately-available input field for location with an autocomplete for the recent entries (instead of listing them as checkboxes)? This would allow the user to immediately enter a location as opposed to making them click twice before they can enter a location.

[anselmbradford]

Unfortunately not. While there are a few complex behaviors that are given directly through HTML, syncing between elements is not one of them. Any syncing of content between elements and attributes requires JS.

In terms of changing the UI and interactivity, that's possible, but that should be moved out of this PR and I would also say the tests should not dictate how the UI behaves. The present UI is based on Linkedin's filtering. There are some technical and layout implications of having the input field immediately present.

Can we merge or close this PR? The thread will remain if there's more to discuss, but I'd rather not have PRs languish for a long time, especially when they're changing only a few lines of code.