♻️🧪 Refactor test cases with SafeElementWrapper querySelector chain to eliminate the unnecessary temporary variable

[balajis-qb]

This PR is regarding enhancement suggested on my previous PR - #5039

Comment Link: #5039 (comment)

As suggested in the linked comment I tried removing the usage of safeQuerySelector or safeQuerySelectorAll usage with SafeElementWrapper method chaining to avoid the unnecessary temporary variables. This is just a test-case refactor.

Contribution checklist

• I have followed the contributing guidelines.
• I have added sufficient test coverage for my changes.
• I have formatted my code with Prettier and checked for linting issues with ESLint for code readability.

[pullrequest]

✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!

What to expect from this code review:

• Comments posted to any areas of potential concern or improvement.
• Detailed feedback or actions needed to resolve issues that are found.
• Turnaround times vary, but we aim to be swift.

---

@balajis-qb you can click here to see the review status or cancel the code review job.

[pullrequest]

PullRequest Breakdown

Reviewable lines of change

+ 66
- 61

100% TSX (tests)

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.85%. Comparing base (c834746) to head (a997af8).
Report is 26 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5062   +/-   ##
=======================================
  Coverage   96.85%   96.85%           
=======================================
  Files          29       29           
  Lines        3343     3343           
  Branches     1390     1403   +13     
=======================================
  Hits         3238     3238           
+ Misses        105      103    -2     
- Partials        0        2    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pullrequest]

This change looks simple enough, I cannot see any issues.

Jacques

---

Reviewed with ❤️ by PullRequest

[pullrequest]

Noting that another PullRequest colleague has already considered this change approvingly: #5039 (comment)

These changes look good, logical, and since they're tests only shouldn't have any security implications. I did note that the linter on PullRequest's end is complaining about some non-null assertions, and provided some mitigation paths for that inline if you're interested. Additionally, since a lot of the code you're adding is similar in structure, I provided a reference implementation for extracting common logic into a function and calling it in your tests.

Dylan F

---

Reviewed with ❤️ by PullRequest

[pullrequest]

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:

describe("TimePicker", () => {
  let datePicker: HTMLDivElement;
  let div: HTMLDivElement;
  let onChangeMoment: Date | undefined;
  let instance: DatePicker | null = null;

  const timeOption: (datePicker: HTMLDivElement): SafeElementWrapper<HTMLDivElement> | undefined = (datePicker) => {
    return new SafeElementWrapper(datePicker)
      .safeQuerySelector(".react-datepicker__time-container')
      .safeQuerySelectorAll("li", MIN_TIME_LI_LEN)[1]? || undefined;
  }

  beforeEach(() => {
    div = document.createElement("div");
  });

  // ...

  it("should allow time changes after input change", () => {
    renderDatePicker("February 28, 2018 4:43 PM");
    setManually("February 28, 2018 4:45 PM");
    if (!instance?.input) {
      throw new Error("input is null/undefined");
    }
    fireEvent.focus(instance.input);
    fireEvent.click(timeOption(datePicker)); // can just use this in other tests within this `describe` block

    expect(getInputString()).toBe("February 28, 2018 12:30 AM");
  });

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

[pullrequest]

ESlint is complaining a bit about your usage of the non-null assertion (!) here.

Looking over the definition and types on SafeElementWrapper and its functions, I can see why you need the bang/!; there's no guarentee that safeQuerySelectorAll's returned array will have an element at index 1, so you do need to do something to make the types work.

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 undefined to fireEvent.click will at the least throw an error at runtime, and TS might throw a typeerror at it depending on what types fireEvent.click accepts.

This same comment applies to your other changes in this file as well, at #188, #203, !221, #258, #277, and #296.

🔸 Giving Information (Important)

Dylan F