RUN-2321-Feat/unit test for activity running indicator #9102

jayas006 · 2024-05-03T21:37:09Z

Is this a bugfix, or an enhancement? Please describe.
This is an enhancement

Describe the solution you've implemented

Verifying that the component renders correctly when the count is 0.
Checking that the component renders and displays the correct count when the count is greater than 0.
Ensuring that the count updates correctly when the updateNowrunning method is called.
Confirming that the component emits the activity-nowrunning-click-action event when clicked.
Checking that the component registers the 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.

smartinellibenedetti

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.

smartinellibenedetti · 2024-05-31T01:46:15Z

...kapp/grails-spa/packages/ui-trellis/src/app/components/activity/activityRunningIndicator.vue

- props: ["eventBus", "displayMode"],
+ props: {
+ eventBus: {
+ type: Object,


Suggested change

type: Object,

type: Object as PropType<typeof EventBus>,

smartinellibenedetti · 2024-05-31T01:52:59Z

rundeckapp/grails-spa/packages/ui-trellis/src/app/components/activity/savedFilters.vue

@@ -44,6 +55,7 @@
 </li>
 </template>
 </dropdown>
+ <span v-else data-test-id="no-filters-message"> No filters available </span>


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.

smartinellibenedetti · 2024-05-31T01:53:29Z

rundeckapp/grails-spa/packages/ui-trellis/src/app/components/activity/savedFilters.vue

@@ -1,14 +1,17 @@
 <template>
 <span>
 <btn
- v-if="hasQuery && (!query || !query.ftilerName)"
+ v-if="hasQuery && (!query || !query.filterName)"


good catch 👍

smartinellibenedetti · 2024-05-31T01:53:44Z

rundeckapp/grails-spa/packages/ui-trellis/src/app/components/activity/savedFilters.vue

+ required: true,
+ },
+ eventBus: {
+ type: Object,


Suggested change

type: Object,

type: Object as PropType<typeof EventBus>,

smartinellibenedetti · 2024-05-31T01:54:45Z

rundeckapp/grails-spa/packages/ui-trellis/src/app/components/activity/savedFilters.vue

+ hasQuery: {
+ type: Boolean,
+ required: true,
+ },


question, are all places using this component passing the prop hasQuery?

smartinellibenedetti · 2024-05-31T02:34:49Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ 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");
+ }
+ });


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 for filterStore.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.

smartinellibenedetti · 2024-05-31T02:35:30Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ 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");
+ });


Would recommend removing this test.

smartinellibenedetti · 2024-05-31T02:37:47Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ });
+ describe("Filter Management", () => {
+ it("updates filters after successful deletion", async () => {
+ await wrapper.vm.loadFilters();


There's no need to call loadFilters here.

smartinellibenedetti · 2024-05-31T02:38:50Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ });
+
+ it("shows 'No filters available' when filters array is empty", async () => {
+ wrapper.vm.filters = [];


Recommend to recreate the wrapper with the correct data instead of mutating it like this.

smartinellibenedetti · 2024-05-31T02:39:03Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ expect(noFiltersMessage).toContain("No filters available");
+ });
+ it("updates visible elements when 'query.filterName' changes", async () => {
+ await wrapper.setProps({ query: { filterName: "Updated Filter" } });


Recommend to recreate the wrapper with the correct data instead of mutating it like this.

jayas006 · 2024-05-31T13:12:47Z

@smartinellibenedetti , Thank you for the feedback. I will go ahead and fix it.

smartinellibenedetti

Left pointers for the mocks, but overall it's the right direction!

smartinellibenedetti · 2024-05-31T21:23:05Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ '[data-test-id="filter-item"]',
+ ).length;
+ // Mock the $confirm to return a resolved promise
+ wrapper.vm.$confirm = () => Promise.resolve();


$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.

smartinellibenedetti · 2024-05-31T21:32:29Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/activity/tests/savedFilters.spec.ts

+ 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);
+ }
+ });


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

jayas006 added 3 commits May 3, 2024 16:15

added unit test for saved Filters

65d783b

unit test for activityRunningIndicator

8721cd7

Refactor ActivityRunningIndicator tests and add unmount event test

619541b

jayas006 changed the title ~~Feat/unit test for activity running indicator~~ RUN-2321-Feat/unit test for activity running indicator May 13, 2024

added beforeUnmount in component

69ac554

jayas006 marked this pull request as ready for review May 15, 2024 00:22

smartinellibenedetti requested changes May 31, 2024

View reviewed changes

Fix most unit tests, one still failing

e372996

smartinellibenedetti reviewed May 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUN-2321-Feat/unit test for activity running indicator #9102

RUN-2321-Feat/unit test for activity running indicator #9102

jayas006 commented May 3, 2024 •

edited

smartinellibenedetti left a comment

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

jayas006 commented May 31, 2024

smartinellibenedetti left a comment

smartinellibenedetti May 31, 2024

smartinellibenedetti May 31, 2024

RUN-2321-Feat/unit test for activity running indicator #9102

Are you sure you want to change the base?

RUN-2321-Feat/unit test for activity running indicator #9102

Conversation

jayas006 commented May 3, 2024 • edited

smartinellibenedetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayas006 commented May 31, 2024

smartinellibenedetti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayas006 commented May 3, 2024 •

edited