-
Notifications
You must be signed in to change notification settings - Fork 900
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
RUN-2321-added unit test for date time picker #9100
Conversation
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.
Left some pointers, but recommend revisiting these tests, as a few of them seem to be redundant.
it("renders correctly when mounted", async () => { | ||
const wrapper = await mountDateTimePicker(); | ||
expect(wrapper.vm.dateString).toBe(moment().format("YYYY-MM-DD")); | ||
expect(wrapper.vm.time).toBeInstanceOf(Date); | ||
}); |
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.
recommend to remove this test
it("sets initial data correctly", async () => { | ||
const wrapper = await mountDateTimePicker(); | ||
expect(wrapper.vm.dateString).toBe(now.format("YYYY-MM-DD")); | ||
expect(wrapper.vm.time).toBeInstanceOf(Date); | ||
}); |
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.
recommend to remove this test
await wrapper.setData({ dateString: newDateString }); | ||
expect(moment(wrapper.vm.time).format("YYYY-MM-DD")).toBe(newDateString); | ||
}); | ||
it("binds v-model and class correctly to date-picker and time-picker", async () => { |
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.
if the test checks that the v-model and classes are correctly applied to the data-picker and time-picker, this test should be checking those components have the classes and the v-model, not the wrapper.
it("applies CSS classes and props correctly", async () => { | ||
const wrapper = await mountDateTimePicker(); | ||
expect(wrapper.find(".date-class").exists()).toBe(true); | ||
expect(wrapper.find(".time-class").exists()).toBe(true); | ||
expect(wrapper.find(".bs-date-picker").exists()).toBe(true); | ||
}); | ||
it("sets correct attributes on date-picker", async () => { | ||
const wrapper = await mountDateTimePicker(); | ||
const datePicker = wrapper.findComponent({ name: "date-picker" }); | ||
expect(datePicker.attributes("modelvalue")).toBe(wrapper.vm.dateString); | ||
expect(datePicker.attributes("class")).toContain(wrapper.vm.dateClass); | ||
expect(datePicker.attributes("clear-btn")).toBe("false"); | ||
}); | ||
it("sets correct attributes on time-picker", async () => { | ||
const wrapper = await mountDateTimePicker(); | ||
const timePicker = wrapper.findComponent({ name: "time-picker" }); | ||
// Convert both values to ISO string format before comparing them | ||
const receivedTime = new Date( | ||
timePicker.attributes("modelvalue") | ||
).toISOString(); | ||
const expectedTime = new Date(wrapper.vm.time).toISOString(); | ||
expect(receivedTime).toBe(expectedTime); | ||
expect(timePicker.attributes("class")).toContain(wrapper.vm.timeClass); | ||
}); |
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.
recommend to combine the test binds v-model and class correctly to date-picker and time-picker
and all the tests from lines 66 to 89 into one single test, avoiding scenarios like checking if the components exist (as mentioned before, if the components don't have a v-if condition attached to it, there's no need to check its existence).
const wrapper = await mountDateTimePicker(); | ||
const newTime = new Date(); | ||
const newTimeFormatted = moment(newTime).format(); | ||
await wrapper.setData({ time: newTime }); |
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.
recommend replacing the wrapper.setData with an emit from timePicker (update:modelValue
), ie:
timePicker.vm.$emit(update:modelValue
, newTime)
if (emitted) { | ||
expect(emitted[0]).toEqual([newTimeFormatted]); | ||
} |
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.
please remove the if, but keep the line 41.
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.
Left 2 small tweaks but once you commit them, feel free to merge this PR.
const timePicker = wrapper.findComponent({ name: "time-picker" }); | ||
// Check v-model and class for date-picker | ||
expect(datePicker.attributes("modelvalue")).toBe(wrapper.vm.dateString); | ||
expect(datePicker.attributes("class")).toContain(wrapper.vm.dateClass); |
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.
suggest to change wrapper.vm.dateClass
(both here and on the timepicker for the written class. It's more readable.
// Check CSS classes and props | ||
expect(wrapper.find(".date-class").exists()).toBe(true); | ||
expect(wrapper.find(".time-class").exists()).toBe(true); | ||
expect(wrapper.find(".bs-date-picker").exists()).toBe(true); |
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.
Can remove those, they are redundant
Fixed |
there are some typescript errors that need to be adjusted in this unit tests |
Thanks Sarah, it's going to be a good practice for me to use TypeScript. |
Is this a bugfix, or an enhancement? Please describe.
This is an enhancement. We've improved the unit tests for the DateTimePicker component to ensure more robust and comprehensive coverage
Describe the solution you've implemented
I have added and updated unit tests for the DateTimePicker component. These tests now cover v-model binding, class binding, attribute setting for both date-picker and time-picker, CSS class and prop existence, correct datetime computation, handling of edge cases like leap years, and accessibility roles
Describe alternatives you've considered
Additional context