-
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 filter #9098
base: main
Are you sure you want to change the base?
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, overall looking good. The component dateFilter could be refactored, once the tests are merged if you'd like to give it a shot, it would be a good practice with vue (the modelValue doesn't need to be destructed into data properties and could be used directly in the template)
modelValue: { | ||
type: Object, | ||
required: 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.
Could you please create a type for modelValue, describing that it has the properties enabled and datetime?
import DateTimePicker from "../dateTimePicker.vue"; | ||
import _ from "lodash"; | ||
let idCounter = 0; | ||
jest.spyOn(_, "uniqueId").mockImplementation(() => `uniqueId_${idCounter++}`); |
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.
you have this spy later in the code, safe to remove this line
it("initializes with correct data", () => { | ||
const wrapper = mountDateFilter(); | ||
expect(wrapper.vm.uid).toBeTruthy(); | ||
expect(wrapper.vm.enabled).toBe(false); | ||
expect(wrapper.vm.datetime).toBe(""); | ||
}); |
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 this test
it("sets uid correctly", () => { | ||
const wrapper = mountDateFilter(); | ||
expect(wrapper.vm.uid).toBe(`uniqueId_${idCounter - 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.
better to check that the input has the right id (which is the uid) instead of checking wrapper.vm.uid
it("renders provided slot content in label", async () => { | ||
const slotContent = "Provided slot content"; | ||
const wrapper = mountDateFilter({ slots: { default: slotContent } }); | ||
await wrapper.vm.$nextTick(); | ||
const label = wrapper.find("label"); | ||
expect(label.text()).toBe(slotContent); | ||
}); |
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.
this test isn't relevant - dateFilter doesn't have logic that would affect the label content (it's always enabled). Suggest to remove it.
it("toggles enabled state when checkbox is clicked", async () => { | ||
const wrapper = mountDateFilter(); | ||
const checkbox = wrapper.find('input[type="checkbox"]'); | ||
await checkbox.setValue(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.
it's always preferable to click then to setValue if possible
await wrapper.vm.$nextTick(); | ||
const emitted = wrapper.emitted("update:modelValue"); | ||
expect(emitted).toBeTruthy(); | ||
if (emitted) { |
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 this if and just leave the line expect(emitted[0]).toEqual([{ enabled: true, datetime: "" }]);
await wrapper.vm.$nextTick(); | ||
const emitted = wrapper.emitted("update:modelValue"); | ||
expect(emitted).toBeTruthy(); | ||
if (emitted) { |
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 if
Yes will do that. Thanks |
export interface ModelValue { | ||
enabled: boolean; | ||
datetime: string; | ||
} |
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.
this type needs to be moved out from the tests as it belongs to the component. To do:
- create "types" folder inside activity folder;
- move the type to a new file named activityTypes;
- rename the type to DateFilterProps;
type: Object, | ||
required: true | ||
} | ||
type: Object as () => ModelValue, |
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.
the correct way to declare the prop is:
type: Object as PropType<ModelValue>
but recommend to use DateFilterProps instead of modelValue
methods: { | ||
toggleEnabled() { | ||
this.enabled = !this.enabled; | ||
}, | ||
}, |
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.
question: why was this added?
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.
Tests are looking good, just need a few adjustments in the component
@smartinellibenedetti , I have followed feedback , went through each pointers and covered everything. |
Is this a bugfix, or an enhancement? Please describe.
This is an enhancement
Describe the solution you've implemented
In the dateFilter.spec.ts file, I implemented tests to verify various aspects of the DateFilter component. These tests are designed to catch potential issues early in the development process and ensure that modifications to other parts of the project do not unintentionally impact the functionality of the DateFilter component.
Describe alternatives you've considered
Additional context