✨Add allow-blocked-end-date flag and e2e test#22029
✨Add allow-blocked-end-date flag and e2e test#22029cvializ merged 4 commits intoampproject:masterfrom
Conversation
9687c8f to
e65ac83
Compare
estherkim
left a comment
There was a problem hiding this comment.
E2E changes LGTM; will defer to others regarding flag implementation
| }); | ||
| }); | ||
|
|
||
| app.use('/date-picker/config.json', (req, res) => { |
There was a problem hiding this comment.
I think @cathyxz split some non test app stuff out into separate files, which I think is a pattern we should follow so that this file doesn't end up being huge as well.
There was a problem hiding this comment.
I'm not sure which non test app stuff you're referring to, since that's what I created this file for, but I'd be glad to do something cleaner in a followup PR.
There was a problem hiding this comment.
Like this:
Line 51 in 98000ef
Basically, move the route logic into a separate file, then just import it from app-test, to keep this file from growing.
There was a problem hiding this comment.
Nice that looks pretty good. I will definitely do that in a followup PR
| * actions instead of text. | ||
| * @enum {string} | ||
| */ | ||
| export const Keys = { |
There was a problem hiding this comment.
This enum name should be singular (i.e. Key)
| } | ||
|
|
||
| if (containsBlocked) { | ||
| if (blockedCount == 1 && this.allowBlockedEndDate_) { |
There was a problem hiding this comment.
function is getting a bit long, would consider extracting out this logic and the above into a function that just checks for blocked days, since it seems to be orthogonal to the rest of the function.
|
|
||
| if (startDate && !endDate && allowBlockedEndDate) { | ||
| return isBlocked && !isSameDay(day, list.firstDateAfter(startDate)); | ||
| } else { |
|
|
||
| // This sequence of commands selects the date one week after the start | ||
| // date since the initial focused cell is the date after the start date. | ||
| await controller.type(null, Keys.ArrowLeft); |
There was a problem hiding this comment.
in tests, not a good idea to rely on the current date. Tests should be (to the extent possible) completely reproducible. For example, your test at some point may go into the next month with the next arrow or down arrow key. It could end up that your test fails at some day in the future where this does not work. While it may work fine in this case, you should use specific dates to test that it works correctly (if that is indeed inspected).
There was a problem hiding this comment.
I agree with you generally, but for a few reasons I think for now this is the way to go:
- E2E tests are designed to execute with as minimal mocking as possible, and spoofing the date in the test is a kind of mocking
- There is currently no API in Puppeteer or Selenium to override the date and would have to be implemented
- The date picker changes the date it opens to depending on the current day and this tests that.
- This sequence of keys is designed to work for any day.
For these reasons I would prefer to keep current behavior at this time.
87a48bd to
fbabdbd
Compare
This comment has been minimized.
This comment has been minimized.
|
PTAL |
|
Thanks for the review 🎉 |
|
Just so you know the flag breaks page validation. |
|
Thanks for reporting, we'll have that fixed shortly |
Implements #17950
/to @cathyxz @sparhami
/cc @estherkim