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

smartinellibenedetti

Please double-check if using MessageBox.confirm is the same behavior as $confirm. If not, I recommend undoing the change. Other than that, this PR just need small changes.

smartinellibenedetti · 2024-06-06T16:30:34Z

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

-          <a role="button" @click="deleteFilter">
+          <a
+            role="button"
+            data-test-id="delete-filter-btn"


if the element is rendered only once, can use data-test-id, if it's rendered multiple times, opt for data-test. Here you can remove the data-test attribute and leave just the data-test-id

Thanks Sarah. Good to know when to use data-test-id and data-test

smartinellibenedetti · 2024-06-06T16:30:48Z

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

+        <li
+          v-for="filter in filters"
+          :key="filter.filterName"
+          data-test-id="filter-item"


here should be data-test instead of data-test-id

smartinellibenedetti · 2024-06-06T16:33:59Z

...s-spa/packages/ui-trellis/src/app/components/activity/tests/activityRunningIndicator.spec.ts

+
+  it("emits 'activity-nowrunning-click-action' event when clicked", async () => {
+    const wrapper = await mountActivityRunningIndicator();
+    wrapper.vm.updateNowrunning(5);


Need to follow the same logic that was done in "renders correctly when count is greater than 0": create the eventBus and trigger an emit from it instead of calling wrapper.vm.updateNowrunning(5);

smartinellibenedetti · 2024-06-06T16:35:00Z

...s-spa/packages/ui-trellis/src/app/components/activity/tests/activityRunningIndicator.spec.ts

+  it("updates count correctly", async () => {
+    const wrapper = await mountActivityRunningIndicator();
+    wrapper.vm.updateNowrunning(10);
+    await wrapper.vm.$nextTick();
+    expect(wrapper.vm.count).toBe(10);
+  });


you can combine this test and the next into a single test - checking that it emits the right event and updates the count correctly

smartinellibenedetti · 2024-06-06T16:48:26Z

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

        .catch(() => {
-          //this.$notify("Delete canceled.");
+          // this.$notify("Delete canceled.");
        });


was this commented before?

Yes it was commented from the starting. I removed it and then added it.

smartinellibenedetti · 2024-06-06T16:48:37Z

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

-      this.$confirm({
+
+      MessageBox.confirm({


Is this change really equivalent?

Is this change really equivalent?

thanks for checking, I probably used it when working on tests but forgot to undo it.

smartinellibenedetti · 2024-06-06T16:54:22Z

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

+      ).length;
+      expect(initialCount).toBeGreaterThan(0);
+      const deleteButton = wrapper.find('[data-test-id="delete-filter-btn"]');
+      if (deleteButton.exists()) {


please remove this if, as explained on another PR.

smartinellibenedetti · 2024-06-06T16:55:55Z

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

+      await wrapper.vm.doDeleteFilter(wrapper.vm.query.filterName);
+      await wrapper.vm.$nextTick();
+      const finalCount = wrapper.findAll('[data-test-id="filter-item"]').length;
+      expect(finalCount).toBeLessThan(initialCount);


Suggest to change toBeLessThan to toBe(initialCount-1) so that the intention is more clear

smartinellibenedetti · 2024-06-06T16:56:33Z

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

+    global: {
+      mocks: {
+        $t: (msg) => msg,
+        $confirm: () => Promise.resolve(),


Since you replace the $confirm for the modal, you don't need to mock it here anymore.

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

jayas006 added 4 commits June 3, 2024 10:12

mocked for updates filters after successful deletion test

8fe4ec4

fixed updates filters after successful deletion test

3fd8938

removed comment

9b5fc4f

Cleaned tests

e8ad5c6

jayas006 mentioned this pull request Jun 4, 2024

RUN-2321- Added unit test for saved Filters #9101

Closed

jayas006 requested a review from smartinellibenedetti June 5, 2024 14:29

smartinellibenedetti requested changes Jun 6, 2024

View reviewed changes

fixed tests according to feedback

c9ba8f5

jayas006 requested a review from smartinellibenedetti June 6, 2024 19:29

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

smartinellibenedetti left a comment

smartinellibenedetti Jun 6, 2024

jayas006 Jun 6, 2024

smartinellibenedetti Jun 6, 2024

smartinellibenedetti Jun 6, 2024

smartinellibenedetti Jun 6, 2024

smartinellibenedetti Jun 6, 2024

jayas006 Jun 6, 2024

smartinellibenedetti Jun 6, 2024

jayas006 Jun 6, 2024

smartinellibenedetti Jun 6, 2024

smartinellibenedetti Jun 6, 2024

smartinellibenedetti Jun 6, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayas006 commented May 3, 2024 •

edited