-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
♻️🧪 Refactor test cases with SafeElementWrapper querySelector chain to eliminate the unnecessary temporary variable #5062
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,12 @@ import React from "react"; | |
import { formatDate, KeyType } from "../date_utils"; | ||
import DatePicker from "../index"; | ||
|
||
import { getKey, safeQuerySelector, safeQuerySelectorAll } from "./test_utils"; | ||
import { | ||
getKey, | ||
SafeElementWrapper, | ||
safeQuerySelector, | ||
safeQuerySelectorAll, | ||
} from "./test_utils"; | ||
|
||
const MIN_TIME_LI_LEN = 2; | ||
|
||
|
@@ -38,12 +43,11 @@ describe("TimePicker", () => { | |
|
||
fireEvent.focus(instance.input); | ||
|
||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.click(lis[1]!); | ||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ESlint is complaining a bit about your usage of the non-null assertion ( Looking over the definition and types on SafeElementWrapper and its functions, I can see why you need the bang/ Per https://typescript-eslint.io/rules/no-non-null-assertion/, you can either tell eslint to skip the rule for this line (it's a test, not user-facing code), or rewrite to satisfy the linter with something like: const timeOption = new SafeElementWrapper(datePicker)
.safeQuerySelector(".react-datepicker__time-container")
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]?
.getElement() || undefined;
timeOption && fireEvent.click(timeOption); You may have other preferences around how to handle this; note that passing This same comment applies to your other changes in this file as well, at #188, #203, !221, #258, #277, and #296. 🔸 Giving Information (Important) |
||
.getElement(); | ||
fireEvent.click(timeOption); | ||
expect(getInputString()).toBe("February 28, 2018 12:30 AM"); | ||
}); | ||
|
||
|
@@ -178,12 +182,13 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Enter)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Enter)); | ||
|
||
expect(getInputString()).toBe("February 28, 2018 12:30 AM"); | ||
}); | ||
|
||
|
@@ -193,12 +198,11 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Space)); | ||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Space)); | ||
expect(getInputString()).toBe("February 28, 2018 12:30 AM"); | ||
}); | ||
|
||
|
@@ -210,14 +214,13 @@ describe("TimePicker", () => { | |
} | ||
|
||
const input = safeQuerySelector(datePicker, "input"); | ||
|
||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Enter)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Enter)); | ||
|
||
await waitFor(() => { | ||
expect(document.activeElement).toBe(input); | ||
|
@@ -230,12 +233,13 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Escape)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Escape)); | ||
|
||
expect(getInputString()).toBe("February 28, 2018 4:43 PM"); | ||
}); | ||
|
||
|
@@ -248,12 +252,13 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Escape)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Escape)); | ||
|
||
expect(onKeyDownSpy).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
|
@@ -266,12 +271,13 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Enter)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Enter)); | ||
|
||
expect(onKeyDownSpy).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
|
@@ -284,12 +290,13 @@ describe("TimePicker", () => { | |
throw new Error("input is null/undefined"); | ||
} | ||
fireEvent.focus(instance.input); | ||
const time = safeQuerySelector( | ||
datePicker, | ||
".react-datepicker__time-container", | ||
); | ||
const lis = safeQuerySelectorAll(time, "li", MIN_TIME_LI_LEN); | ||
fireEvent.keyDown(lis[1]!, getKey(KeyType.Space)); | ||
|
||
const timeOption = new SafeElementWrapper(datePicker) | ||
.safeQuerySelector(".react-datepicker__time-container") | ||
.safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]! | ||
.getElement(); | ||
fireEvent.keyDown(timeOption, getKey(KeyType.Space)); | ||
|
||
expect(onKeyDownSpy).toHaveBeenCalledTimes(1); | ||
}); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on your preferences and repo contribution guidelines (some folks have a strong preference for specs to be verbose rather than DRY), you could extract the common logic for this into a separate private function and re-use it throughout your tests, like:
Note that I'm just writing this in a comment box, not in a live coding environment—you may have to check and massage the type signatures a bit 😅
🔹 Code Reuse (Nice to have)
Dylan F