Adding specs (mostly refactoring of helper-methods)

[bpurinton]

This project is again a re-factoring (see this PR) so the tests are largely unchanged for now.

But there are some additional things here:

• search for <div class="card">
• search for Font Awesome icon trash can
• presence of bootstrap navbar
• presence of padding with bootstrap class="container"
• basic Devise signup
• before_action :authenticate_user! from devise

Things that are not yet tested for but could be added at a later point:

• other Devise steps like logging out and logging in
• use of partials for view templates
• use of form partials with variables
• use of link_to helper methods in navbar
• presence of private controller actions, like movie_params to permit new columns
• presence of before_action in controller to set_movie for certain actions

[bpurinton]

@jelaniwoods Similar to my comment here, I'm in favor of merging these basic feature tests and converting my un-checked list above to an issue then making separate PRs (eventually) for creating (or not) those specs.

[jelaniwoods]

@bpurinton nice work on this!

The main comment I left is about a spec title saying padding when the spec checks for margin. Let me know what you think about the other comments

[jelaniwoods]

Since you're testing for a specific type of bootstrap navbar, it might be nice to specify that here. Maybe something like this:

Suggested change

	it "has a bootstrap navbar", points: 1 do
	it "has a large, light-themed bootstrap navbar", points: 1 do

What do you think?

[bpurinton]

I agree, changed this

[jelaniwoods]

The spec title and error message are inaccurate. You're testing for the mt-3 class which adds margin not padding.

[bpurinton]

Ah, yes, thanks. Changed the title and message.

[jelaniwoods]

This is kind of a nit pick, but the custom error message only needs to describe the current expectation, it doesn't need to describe the steps before that expectation, if that makes sense?

Expected to fill in the new movie form, click 'Create Movie', and be redirected to the movie index

This behavior is already tested before the expectation, and if any of those steps fail the custom error message won't be displayed so there isn't much value in putting more context about the steps into the custom error message.

As an alternative, you can make the spec title more descriptive:

it "has a form that creates a movie record", point: 1 do

(If you want to test that a movie was created, you can compare the number of movie records before and after the form submits.)

Or If you want to test the presence of a notice message then I might reword to something like

it "displays a notice flash message after creating movie", point: 1 do

Thoughts?

[bpurinton]

I appreciate all these comments! Helping me a lot understand the testing strategy. I'm working on separating this into two tests: 1. is a movie created; 2. is the user shown a success flash message.

[bpurinton]

EDIT / UPDATE: I implemented a strategy for handling devise in the most recent commit. This is important and will be re-used in Photogram Industrial tests.

@jelaniwoods I made the edits you suggested, but another thing occurred to me. The last step in the project is installing devise and the last suggested step there is adding before_action :authenticate_user! to the ApplicationController. Adding that breaks most of the tests. Should I just wrap all of my current tests (besides the users/sign_up) inside a nested author login block? I know your testing strategy is to avoid any nesting, so not sure the cleanest way to check for the before_action and have a user signed in before doing any of the other tests.

On the other hand, not so bad to just add a before do sign in step in every describe block, e.g.:

describe "The /movies page" do
  let(:user) { User.create(email: "alice@example.com", password: "password") }

  before do
    visit new_user_session_path

    fill_in "Email", with: user.email
    fill_in "Password", with: user.password
    click_button "Log in"
  end

  it "can be visited", points: 1 do
    visit "/movies"

    expect(page.status_code).to be(200),
      "Expected to visit /movies successfully."
  end

[jelaniwoods]

I made the edits you suggested, but another thing occurred to me. The last step in the project is installing devise and the last suggested step there is adding before_action :authenticate_user! to the ApplicationController. Adding that breaks most of the tests. Should I just wrap all of my current tests (besides the users/sign_up) inside a nested author login block? I know your testing strategy is to avoid any nesting, so not sure the cleanest way to check for the before_action and have a user signed in before doing any of the other tests.

On the other hand, not so bad to just add a before do sign in step in every describe block, e.g.:

🤔 If the idea is that they only add users at the very end and they should still be able to pass some tests before that step, then you could create a method that signs a user in and then only run it if the student has created a user model.

Something like this:

  it "can be visited", points: 1 do
    sign_in_user if user_model_exists?
    visit "/movies"

    expect(page.status_code).to be(200),
      "Expected to visit /movies successfully."
  end

you can define sign_in_user and user_model_exists? at the bottom of the spec file

def sign_in_user
  user = User.create(email: "alice@example.com", password: "password")
  visit new_user_session_path

  fill_in "Email", with: user.email
  fill_in "Password", with: user.password
  click_button "Log in"
end

def user_model_exists?
  Object.const_defined?("User")
end

Then, the movie related tests should pass with or without the authenticate_user! before action. This is definitely a hacky solution. What do you think?

[bpurinton]

@jelaniwoods I didn't do that in my most recent commit, but what I did do in my most recent commit (testing for redirect and adding before do log in blocks) does work. FYI I'm testing everything on this branch. If both solutions (yours and mine) are a bit "hacky" is one better than the other?

Update: Wait, I see why yours is better. It allows the other tests to pass before adding devise and User accounts (last step of lesson).

[jelaniwoods]

what I did do in my most recent commit (testing for redirect and adding before do log in blocks) does work... If both solutions (yours and mine) are a bit "hacky" is one better than the other?

@bpurinton
Both solutions will attempt to sign in a user before visiting the test page. Your solution will fail when all the movies RCAV has been created but the student hasn't created a user model (or added Devise). In the same situation my solution will pass. I think it might be preferable to let some of the tests pass with or without Devise, since it's the last set of instructions in the READEME and we probably still want to know how much progress they've completed on the assignment.

With your solution, all tests will fail until Devise is installed and user model created. This makes it harder for use to know their progress since it's the last step.

Does that help clarify?

If Devise is one of the first steps then it shouldn't matter which solution is used.

[bpurinton]

@jelaniwoods Okay, maybe last little review here before merging. Using your devise testing strategy and before blocks to (maybe) sign in a user.

[jelaniwoods]

@bpurinton everything looks good to me! I left a small comment, feel free to ignore.

[jelaniwoods]

When Devise isn't installed this will fail since new_user_session_path isn't defined. That won't make the custom error message display though, since the expectation never ran.

If your intention was that the custom error message would be visible to students who haven't created users, then you should use the String path instead. Something like:

    expect(current_url).to eq("/users/sign_in"),

I also buy the idea that since the spec title already mentions Devise, student could infer that they need devise to make the test pass w/out the help of the error message. It's up to you if you want to make this change.

[bpurinton]

I also buy the idea that since the spec title already mentions Devise, student could infer that they need devise to make the test pass w/out the help of the error message. It's up to you if you want to make this change.

Yeah I considered my use of the helper method there, but that's why I include Devise in the spec. The assumption is that they should have added devise and installed it, which will provide those helper methods.

Merging! Thanks for all your help!

[jelaniwoods]

I remember now that students tend to work top down from the list of specs, so placing the Devise-required specs at the bottom will probably eliminate some friction. It's not a big deal though.

[bpurinton]

Ah, sure. Just going to make that change directly on main.