-
Notifications
You must be signed in to change notification settings - Fork 899
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-Feat/unit test for activity running indicator #9102
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.
Good job selecting what to test, there's some cleanup to do, but overall looking good. Please let me know if there are any doubts.
props: ["eventBus", "displayMode"], | ||
props: { | ||
eventBus: { | ||
type: Object, |
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.
type: Object, | |
type: Object as PropType<typeof EventBus>, |
@@ -44,6 +55,7 @@ | |||
</li> | |||
</template> | |||
</dropdown> | |||
<span v-else data-test-id="no-filters-message"> No filters available </span> |
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.
I would recommend removing this line, as it is confusing to read "no filters available" when it's still possible to filter (it's just that there are no saved filters), besides the fact that when writing tests, components shouldn't have their behavior altered.
@@ -1,14 +1,17 @@ | |||
<template> | |||
<span> | |||
<btn | |||
v-if="hasQuery && (!query || !query.ftilerName)" | |||
v-if="hasQuery && (!query || !query.filterName)" |
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.
good catch 👍
required: true, | ||
}, | ||
eventBus: { | ||
type: Object, |
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.
type: Object, | |
type: Object as PropType<typeof EventBus>, |
hasQuery: { | ||
type: Boolean, | ||
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.
question, are all places using this component passing the prop hasQuery?
if (initialCount > 0) { | ||
console.log("Before delete:", wrapper.vm.filters); | ||
// Mock the deleteFilter method | ||
wrapper.vm.deleteFilter = (filterName) => { | ||
const index = wrapper.vm.filters.findIndex( | ||
(filter) => filter.filterName === filterName, | ||
); | ||
if (index !== -1) { | ||
wrapper.vm.filters.splice(index, 1); | ||
} | ||
}; | ||
await wrapper.vm.deleteFilter(wrapper.vm.filters[0].filterName); | ||
await wrapper.vm.$nextTick(); | ||
console.log("After delete:", wrapper.vm.filters); | ||
|
||
expect(wrapper.vm.filters.length).toBeLessThan(initialCount); | ||
} else { | ||
throw new Error("No filters available to delete"); | ||
} | ||
}); |
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.
A few things here:
-
It's understandable if using if/else helps during the development of tests, but please avoid keeping them once tests are finished. Unless components have random functionalities, unit tests should be deterministic, which means that they use the same data every time and will always output the same results, therefore, there's no point in using if/else on them.
-
instead of triggering wrapper.vm.deleteFilter, trigger the user interaction that will call the method deleteFilter;
-
Instead of mocking the delete filter, mock the $confirm to return a resolved promise and either adjust
filterStore.removeFilter
to return the correct array of filters, OR before the click to trigger deleteFilter, do a mockReturnValue forfilterStore.loadForProject
to return an object with the filter property; -
lastly, instead of checking if filters.length is less than initialCount, it would be better to count the number of links rendered, from this block:
<li v-for="filter in filters" :key="filter.filterName">
<a role="button" @click="selectFilter(filter)">
{{ filter.filterName }}
<span v-if="query && filter.filterName === query.filterName"
>√</span
>
</a>
</li>
To count them, it's possible to add a "data-test" attribute to the tag, like this example here and then do the same logic of an initial count before removal, and a comparison of the numbers afterward.
it("shows 'No filters available' when filters array is empty", async () => { | ||
wrapper.vm.filters = []; | ||
await wrapper.vm.$nextTick(); | ||
|
||
const noFiltersMessage = wrapper | ||
.find('[data-test-id="no-filters-message"]') | ||
.text(); | ||
expect(noFiltersMessage).toContain("No filters available"); | ||
}); |
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.
Would recommend removing this test.
}); | ||
describe("Filter Management", () => { | ||
it("updates filters after successful deletion", async () => { | ||
await wrapper.vm.loadFilters(); |
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.
There's no need to call loadFilters here.
}); | ||
|
||
it("shows 'No filters available' when filters array is empty", async () => { | ||
wrapper.vm.filters = []; |
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 recreate the wrapper with the correct data instead of mutating it like this.
expect(noFiltersMessage).toContain("No filters available"); | ||
}); | ||
it("updates visible elements when 'query.filterName' changes", async () => { | ||
await wrapper.setProps({ query: { filterName: "Updated Filter" } }); |
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 recreate the wrapper with the correct data instead of mutating it like this.
@smartinellibenedetti , Thank you for the feedback. I will go ahead and fix it. |
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 pointers for the mocks, but overall it's the right direction!
'[data-test-id="filter-item"]', | ||
).length; | ||
// Mock the $confirm to return a resolved promise | ||
wrapper.vm.$confirm = () => Promise.resolve(); |
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.
$confirm should be mocked the same way $t is mocked, which is inside the mount method (in this case the mountSavedFilters) via the global.mocks property.
ActivityFilterStore.prototype.removeFilter = jest | ||
.fn() | ||
.mockImplementation((filterName) => { | ||
const index = wrapper.vm.filters.findIndex( | ||
(filter) => filter.filterName === filterName | ||
); | ||
if (index !== -1) { | ||
wrapper.vm.filters.splice(index, 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.
Might be needed to tweak this mock as well, to either match the format of the editProjectFileService in EditProjectFile.spec.ts
OR follow one of the approaches described here: https://www.mikeborozdin.com/post/changing-jest-mocks-between-tests
Is this a bugfix, or an enhancement? Please describe.
This is an enhancement
Describe the solution you've implemented
updateNowrunning
method is called.activity-nowrunning-click-action
event when clicked.activity-nowrunning-count
event on mount.-Ensuring that the component unregisters the
activity-nowrunning-count
event on unmount.Describe alternatives you've considered
Additional context
These unit tests were created to improve the reliability and maintainability of the
ActivityRunningIndicator.vue
component by automating the verification of its key behaviors. This helps ensure that the component works as expected and reduces the risk of introducing bugs when making changes to the component in the future.