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.

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

grader buggy behavior for director (sad path) #24

Open
bill-auger opened this issue Jul 12, 2016 · 14 comments
Open

grader buggy behavior for director (sad path) #24

bill-auger opened this issue Jul 12, 2016 · 14 comments

Comments

@bill-auger
Copy link
Member

bill-auger commented Jul 12, 2016

i am confused about one of the grader specs for " Scenario: can't find similar movies if we don't know director (sad path)"

Then I should either be on the home page or the RottenPotatoes home page
  expected "/movies/director" to satisfy block (RSpec::Expectations::ExpectationNotMetError)
  ./features/step_definitions/saas_web_steps.rb:3:in `/^(?:|I )should either be on (.+) or (.+)$/'
  ./required_features/director.feature:33:in `Then I should either be on the home page or the RottenPotatoes home page'

nothing of this is mentioned on the homework spec - it literally says you can handle the sad path anyway you like but then the grader constrains implementation to the '/' route

  • there is a default route present in the skeleton that routes root to /movies (which has no effect because public/index.html is also present
  • this requires an extra redundant path in features/support/paths.rb to "the RottenPotatoes home page"
  • this also requires a switch for the 'by-director' path in features/support/paths.rb that returns a different URL when no director or else the 'by-director' page must be accessible at '/'
  • using '/' as an error page (or 'find-by-director' page) is not at all sensible and is encouraging poor design practice

now here the strange part ... after implementing those kludges above to compensate for this "RottenPotatoes home page" requirement - the grader spec then changes:

Then I should be on the Similar Movies page for "Alien"
And I should see "'Alien' has no director info"
expected to find text "'Alien' has no director info" in
    ".... Welcome aboard .... Set up a default route and remove public/index.html ....

i set the "Similar Movies" path to '/' in order to inject that failure above because the grader for this assignment and the previous one do not display any of the specs unless they fail and i can not find where these specs are defined - so troubleshooting has been tricky

related forum thread:
https://courses.edx.org/courses/course-v1:BerkeleyX+CS169.1x+3T2015SP/discussion/forum/i4x-edx-templates-course-Empty/threads/57806dfdd5dc30052a0001a8

@tansaku
Copy link
Contributor

tansaku commented Jul 14, 2016

okay, so this relates to this scenario:

Scenario: can't find similar movies if we don't know director (sad path)
  Given I am on the details page for "Alien"
  Then  I should not see "Ridley Scott"
  When  I follow "Find Movies With Same Director"
  Then  I should be on the home page
  And   I should see "'Alien' has no director info"

I cannot see anywhere that it says "you can handle the sad path anyway you like", what it says is

The third (scenario) handles the sad path, when the current movie has no director info but we try to do "Find with same director" anyway.

@tansaku
Copy link
Contributor

tansaku commented Jul 14, 2016

it also says:

It's up to you to decide whether you want to handle the sad path of "no director" in the controller method or in the model method, but you must provide a test for whichever one you do. Remember to include the line require 'rails_helper' at the top of your *_spec.rb files.

but that's about how you implement things in the controller or the model - the feature constrains the sad path to return to the home page on failure

@RobertStroud
Copy link
Contributor

I agree that the feature constrains the sad path to return to the home page on failure, but this isn't a constraint on whether you handle the sad path in the controller method or the model method. Either approach is reasonable, but regardless of whether the controller or the model detects the missing director info, you need to end up on the home page.

The difficulty is that "the home page" is defined as "/" in "features/support/paths.rb" - my solution was to change the definition of "the home page"... :-)

@bill-auger
Copy link
Member Author

bill-auger commented Jul 14, 2016

ok but that is only one part of the issue - after the path to "the home page or the RottenPotatoes home page" is sorted out then the spec changes and it no longer uses that path but then expects "Then I should be on the Similar Movies page for "Alien""

what i meant by "handling the sad path anyway you like" was that the instructions do not specify a particular route where the director page (nor the sad path endpoint) must be accessible

but this can not be the home page - the home page is '/' which is the public/index.html in the grader's local copy - so for the grader can see "'Alien' has no director info" on "the home page" is to hard-code that literal text into public/index.html and include it in the archive - but as i just mentioned that still does not work because the grader then changes the URI it expects the sad path to end on "the Similar Movies page" which IMHO is more sensible

@RobertStroud
Copy link
Contributor

Hmm - if I've understood correctly, you're saying that there's a difference between the Cucumber tests provided with the homework and the Cucumber tests that the grader uses...

This may be causing some confusion - for example, @tansaku is quoting the homework tests whereas you're quoting the grader tests.

If the two sets of tests are inconsistent, then I agree there's a problem with the grader.

But "public/index.html" is still present in the homework too, which is why I ran into the issue whilst working on my homework.

Unfortunately, I can't test how the grader reacts to my homework at the moment because the grader doesn't seem to be available...

@bill-auger
Copy link
Member Author

bill-auger commented Jul 14, 2016

@RobertStroud - i am not referring to any of the homework tests - this is entirely related to the auto-grader - this issue probably should have been posted on the grader repo to avoid confusion

this step initially fails:

"Then I should either be on the home page or the RottenPotatoes home page"

after you define a path for "the RottenPotatoes home page" then the grader no longer uses that path but instead changes the step to:

"Then I should be on the Similar Movies page for "Alien""

@tansaku
Copy link
Contributor

tansaku commented Jul 15, 2016

taking some of the other points in turn:

there is a default route present in the skeleton that routes root to /movies (which has no effect because public/index.html is also present

that is true

this requires an extra redundant path in features/support/paths.rb to "the RottenPotatoes home page"

yes, not ideal

this also requires a switch for the 'by-director' path in features/support/paths.rb that returns a different URL when no director or else the 'by-director' page must be accessible at '/'

I'm not sure I agree - at least I didn't need an additional by-director path

using '/' as an error page (or 'find-by-director' page) is not at all sensible and is encouraging poor design practice

I'm not sure I agree. I think the idea is to use a flash message. Whether after a search for movies by the same director you should see a custom error page, or be redirected to the main list with a flash error message seems more a matter of taste to me ...

the problem here is that the grader still has 'public/index.html' present in it's app so any root override in routes.rb is ignored - the only way i have found to get 100% is to put literal text in "'Alien' has no director info" and include it in the archive

For me I just added to the flash, and updated the paths.rb file like so:

  def path_to(page_name)
    case page_name

    when /^the (RottenPotatoes )?home\s?page$/ then '/movies'
    when /^the movies page$/ then '/movies'
    when /^the edit page for "(.*)"$/ then edit_movie_path(Movie.find_by(title: $1))
    when /^the details page for "(.*)"$/ then movie_path(Movie.find_by(title: $1))
    when /^the Similar Movies page for "(.*)"$/ then movies_path

@tansaku
Copy link
Contributor

tansaku commented Jul 15, 2016

Also I've been looking at the grader files and these are the feature files it uses:

Feature: search for movies by director

  As a movie buff
  So that I can find movies with my favorite director
  I want to include and serach on director information in movies I enter

Background: movies in database

  Given the following movies exist:
  | title        | rating | director     | release_date |
  | Star Wars    | PG     | George Lucas |   1977-05-25 |
  | Blade Runner | PG     | Ridley Scott |   1982-06-25 |
  | Alien        | R      |              |   1979-05-25 |
  | THX-1138     | R      | George Lucas |   1971-03-11 |

Scenario: add director to existing movie
  When I go to the edit page for "Alien"
  And  I fill in "Director" with "Ridley Scott"
  And  I press "Update Movie Info"
  Then the director of "Alien" should be "Ridley Scott"

Scenario: find movie with same director
  Given I am on the details page for "Star Wars"
  When  I follow "Find Movies With Same Director"
  Then  I should be on the Similar Movies page for "Star Wars"
  And   I should see "THX-1138"
  But   I should not see "Blade Runner"

Scenario: can't find similar movies if we don't know director (sad path)
  Given I am on the details page for "Alien"
  Then  I should not see "Ridley Scott"
  When  I follow "Find Movies With Same Director"
  Then  I should either be on the home page or the RottenPotatoes home page
  And   I should see "'Alien' has no director info"

so there is an inconsistency between that paste bin of the features and what the grader is using, which is potentially confusing. We also don't need that public/index.html page ...

anyway, I'm just going to roll out a change to the grader that should provide more details on any errors in the last two assignments

@tansaku
Copy link
Contributor

tansaku commented Jul 15, 2016

@RobertStroud - thanks for your input - much appreciated - what makes you say you think the grader is unavailable?

@RobertStroud
Copy link
Contributor

On 15 Jul 2016, at 11:53, Sam Joseph [email protected] wrote:
@RobertStroud - thanks for your input - much appreciated - what makes you say you think the grader is unavailable?

I tried to submit my coursework several times yesterday afternoon (UK time) and was told that the grader was unavailable - the message said please try later.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@tansaku
Copy link
Contributor

tansaku commented Jul 18, 2016

that's strange @RobertStroud - the grader appears live - I've just submitted through edX and got a result. Sometimes edX has network issues that prevents us from contacting our grader, but it appears okay at the moment - I would test with your zip file, but annoyingly HPU email removed it from your email to me.

See #23

@enkaba got the same message

Unable to deliver your submission to grader (Reason: cannot connect to server). Please try again later.

but was able to submit after ... that's an edX timeout that we do have occasionally. Basically it means that edX can't talk to the grader due to connection issues ...

@tansaku
Copy link
Contributor

tansaku commented Jul 18, 2016

I'm going to now test all the zip files I can get my hands on to see if there is any other problem that I have power to correct :-)

@RobertStroud
Copy link
Contributor

@tansaku - yes, the grader is live, but it rejects submissions that are too large (> 1MB?). Unfortunately, the error message is misleading - it suggests a temporary connection issue, but it's actually a permanent problem.

Several students had this problem - here's one of the discussion threads:

https://courses.edx.org/courses/course-v1:BerkeleyX+CS169.1x+3T2015SP/discussion/forum/i4x-edx-templates-course-Empty/threads/578b1e09af36f3051300031c

@tansaku
Copy link
Contributor

tansaku commented Jul 19, 2016

thanks @RobertStroud that's good to know - will try and find some longer term solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants