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

Flaky spec: Polls Booth & Website Already voted on booth cannot vote on website #1349

Conversation

iagirre
Copy link

@iagirre iagirre commented Mar 9, 2018

References

Objectives

Explain why the test is flaky, or under which conditions/scenario it fails randomly

(This explanation is the same as in #1342 and #1343 PRs because the problem is exactly the same).

The problems was that the test was not reaching to the residence verification page, so it couldn't find the select box for the DNI (and that's the error reported in the issue). To figure out what was going on here, I studied the flow the test follows so that I could find the scenarios where it would crash, and, after doing it, step by step, I determined that the test will fail when:

  • There are more than 1 officer_assignments (booths) for this user, so the page will redirect to the select booth page.
  • There aren't any officer_assignments, so that the user can't even reach the target page (it redirects to the root).

The code that determines that is:

# app/controller/officing/base_controller.rb
	[...]

	def load_officer_assignment
      @officer_assignments ||= current_user.poll_officer.officer_assignments.where(date: Time.current.to_date)
    end

    def verify_officer_assignment
      if @officer_assignments.blank?
        redirect_to officing_root_path, notice: t("officing.residence.flash.not_allowed")
      end
    end

    def verify_booth
      return unless current_booth.blank?
      booths = todays_booths_for_officer(current_user.poll_officer)
      case booths.count
      when 0
        redirect_to officing_root_path
      when 1
        session[:booth_id] = booths.first.id
      else
        redirect_to new_officing_booth_path
      end
    end

    [...]

    def todays_booths_for_officer(officer)
      officer.officer_assignments.by_date(Date.today).map(&:booth).uniq
    end

After trying to reproduce it (without success), I saw the new piece of information about the probable failing hours, and I realized that the problem could be in the moment of creating the objects. If the officer_assignment are created at 23:59:59 and the rest of the test is executed after 00:00:00, the dates for the objects and the Date.current (used to check if there are any shifts today) won't be the same, because the shift will be for, lets say, 07/03/2018 and Date.current will be 08/03/2018.

To prove that, I forced in the factory the dates of the officer_assignments to an earlier date, and it failed in the exact same way as the reported ones.

Explain why your PR fixes it

I stubbed the Date and Time clases to force them to give me a date I can controll. I set that date (01/01/2018) to the objects that depend on it (poll_shift, officer_assignment and polls). This way, it doesn't matter when the test is executed, for it will always be the same date.

Visual Changes (if any)

There aren't, is a flaky.

Notes

  • In this case, I only stubbed them in the context called "Booth & Website", not the whole spec, because this is the one that has problems with dates.

Copy link

@MariaCheca MariaCheca left a comment

Choose a reason for hiding this comment

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

Wonderful @iagirre! Everything looks good, I just left a couple of comments, about a tiny detail :)

scenario 'Already voted on booth cannot vote on website', :js do

create(:poll_shift, officer: officer, booth: booth, date: Date.current, task: :vote_collection)
create(:poll_shift, officer: officer, booth: booth, date: '2018-01-01', task: :vote_collection)

Choose a reason for hiding this comment

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

Couldn't this date: '2018-01-01' be replaced by a Date.current? As you specify in the before block, the Date.current will always return that same date.

booth_assignment = create(:poll_booth_assignment, poll: poll, booth: booth)
create(:poll_officer_assignment, officer: officer, booth_assignment: booth_assignment)
create(:poll_officer_assignment, officer: officer, booth_assignment: booth_assignment, date: '2018-01-01')

Choose a reason for hiding this comment

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

Same as in my first comment, is there a particular reason why the date attribute is not set using the Date.current method?

@bertocq
Copy link

bertocq commented Mar 21, 2018

@iagirre can you provide us an answer for Maria's comments? Thanks!

@iagirre iagirre force-pushed the 1208-flaky-already-voted-on-booth-cannot-vote-on-website branch from b0e3a36 to aa4343d Compare March 21, 2018 16:28
@iagirre iagirre force-pushed the 1208-flaky-already-voted-on-booth-cannot-vote-on-website branch from aa4343d to da61e40 Compare March 21, 2018 16:29
@iagirre
Copy link
Author

iagirre commented Mar 21, 2018

I'm sorry, I made changes to other PRs but I skiped this one.
Yes, as Maria pointed out, when the Date.current function is mocked It can be used instead of hardcoding the date in the objects creation.
I've updated the code already.

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

Successfully merging this pull request may close these issues.

3 participants