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

Fix invalid date selection issue for e2e_tests #2341

Merged
merged 6 commits into from Jul 8, 2019

Conversation

5 participants
@mkrump
Copy link
Contributor

commented Jul 2, 2019

Description

The cypress test is selecting a day from the date picker for the current month. This will fail when the date is a weekend or holiday. Assuming that it's not possible to hard code the date and enter the date and that have to interact via the date picker (as I think that would probably be the easiest way to resolve)?

Reviewer Notes

Wasn't sure the best way to resolve this. Also, not really sure what's been tried here, so if you all could weigh in that would be appreciated. Two commits with two different approaches:

  • 34b5ab1...e9188bc make a call to the available_move_dates endpoint and uses the result. Maybe overly complicated for what we're trying to do here, but uses our holiday calendar / don't have to maintain another calendar.
  • 34b5ab1...affb732 uses moment-business-days. This is simpler, but requires that you add a holiday calendar, that we'd have to update going forward, whereas that's already being done in the available_move_dates endpoint.

*** Updated ***
Was directed to this story. This specific test was failing due to the shipments model validation rules. Adding a third option (remove the actual delivery date DateIsWorkday validation rule), but didn't have the context to know if that was correct to remove it entirely.

Setup

make e2e_test

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@mkrump mkrump added the 🦇team label Jul 2, 2019

@mkrump mkrump requested review from chrisgilmerproj, lynzt and sarboc Jul 2, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

I prefer the use of the available_move_dates endpoint because it keeps our maintenance burden low for the holiday stuff.

@mkrump mkrump requested a review from donaldthai Jul 2, 2019

@donaldthai
Copy link
Contributor

left a comment

For the holidays, we could use another library to determine it: https://github.com/18F/us-federal-holidays

@mkrump mkrump requested review from tinyels and chrisgilmerproj Jul 2, 2019

@codecov

This comment has been minimized.

Copy link

commented Jul 2, 2019

Codecov Report

Merging #2341 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2341      +/-   ##
==========================================
+ Coverage   59.71%   59.73%   +0.03%     
==========================================
  Files         261      262       +1     
  Lines       14702    14732      +30     
==========================================
+ Hits         8778     8800      +22     
- Misses       4902     4909       +7     
- Partials     1022     1023       +1
@lynzt

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

If we already have a way to calculate holidays, I'd use that... As Matt said, prevents us from having to maintain another holiday calendar.
However, w/ the update of the business rules seems like we could get rid of testing for weekend dates all together since the TSP can choose a weekend.
Question: Are TSPs allowed to choose a date that might fall on a holiday? Do we still need a check for that?

@tinyels

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

I'm of two minds about this:
The pragmatic mind says that we are moth-balling HHGs and should do the least amount of work possible to get this test passing. That is, we could remove the verification entirely.
The perfectionist says that we want to check for weekends and holidays if the request is from a service member. It also say that the business rules for pickup and packing date should be corrected too.

For this story, I prefer the available_move_dates, since a) you have already done the work and b) it will be resilient to any changes we make to that business logic going forward.

@@ -4,7 +4,7 @@ FROM cypress/base:10
# We use `npm install` here because you can't selectively `yarn install` packages and we don't care about a lockfile and
# `yarn add` won't read from our existing package.json
COPY package.json /package.json
RUN npm install --save-dev mocha mocha-multi-reporters mocha-junit-reporter
RUN npm install --save-dev mocha mocha-multi-reporters mocha-junit-reporter moment

This comment has been minimized.

Copy link
@mkrump

mkrump Jul 3, 2019

Author Contributor

had to add moment to the cypress container since using it within cypress now.

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

thanks for the feedback everyone. seems like the consensus is to give the available_move_dates approach a shot. updated PR to use that.

@mkrump mkrump marked this pull request as ready for review Jul 3, 2019

@mkrump mkrump merged commit 8bb0651 into master Jul 8, 2019

19 checks passed

ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 79dab02...eb0c90f
Details
codecov/project/go 59.57% (+0.03%) compared to 79dab02
Details

@mkrump mkrump deleted the mk-167026270-date-issue-e2e branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.