RUN-2307 unit tests for edit project file #9084

jayas006 · 2024-04-25T21:16:42Z

Is this a bugfix, or an enhancement? Please describe.
This is an enhancement. I have added unit tests for the EditProjectFile.vue component to ensure its functionality and behavior are as expected

Describe the solution you've implemented
I have written a series of unit tests using Jest and Vue Test Utils. These tests cover various scenarios including:

Rendering the correct title based on the filename.
Rendering file content when the getFileText method returns successfully.
Handling failure when the getFileText method fails.
Handling failure when a user edits the file and fails to save it.
Handling success when a user edits the file and saves it.
Displaying admin-specific messages and configuration links when the user is an admin.
Displaying non-admin specific messages when the user is not an admin.
Not allowing non-admin users to save or edit the file.
Rendering the Ace Editor.
file content does not change when the user cancels the edit

Describe alternatives you've considered

Additional context
These tests will help ensure that the component works correctly and continues to do so as the application evolves.

jayas006 · 2024-04-29T17:10:13Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

-    const aceEditor = wrapper.find('[data-test-id="ace-editor"]');
-    expect(aceEditor.exists()).toBe(true);
-  });
+  //TODO: working on this one


@smartinellibenedetti Can you please look at it? This one not working. Thanks

When you trigger actions that require the UI to re-render, remember to do a:

await wrapper.vm.$nextTick();

before trying to assert the values

smartinellibenedetti · 2024-04-29T19:41:14Z

rundeckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.vue

@@ -45,25 +45,30 @@
                  </details>
                </div>
                <ace-editor
+                  v-if="authAdmin"


Important, the behaviour of the components shouldn't change while writing tests, therefore please undo the changes on lines 48 and 68.

When the user doesn't have permissions to create/edit files, the component will render strings (whose logic is defined inside the block that starts with <template v-if="displayConfig.includes('none')">) and the tests should confirm that the right strings are being rendered in such cases.

smartinellibenedetti

A few things to fix/adjust, but overall looking good.

smartinellibenedetti · 2024-05-06T19:04:33Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+  });
+
+  it("renders file content when getFileText method returns successfully", async () => {
+    expect(wrapper.vm.fileText).toBe("sample file content");


Could you please change this test to assert instead that the HTML rendered by ace-editor has this string inside of it?

smartinellibenedetti · 2024-05-06T19:20:53Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+  it("does not allow non-admin user to edit the file", async () => {
+    await mountEditProjectFile({ authAdmin: false });
+    expect(wrapper.find('[data-test-id="my-ace-editor"]').exists()).toBe(false);


Please remove this test as the original component doesn't have the behavior of not rendering the editor if the user isn't an admin

smartinellibenedetti · 2024-05-06T19:37:37Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    wrapper.vm.createProjectHomeLink = jest.fn(() => {
+      wrapper.vm.fileText = wrapper.vm.originalFileText;
+    });


When I wrote the ticket description, I was aware that if the user pressed cancel, the text wouldn't be saved and was under the impression that the component would revert back to the the original text, but turns out that upon clicking cancel, the component is triggering a navigation.

So best thing to do is remove lines 146-148, and you trigger click on the cancel button, await the next tick and assert the document location. If the document won't be easy to test, you can replace this line in the component:

document.location = url("project/" + this.project + "/home").href;

to:

window.location = url("project/" + this.project + "/home").href;

document.location and window.location will have the same outcome, but it's easier to test the window.

And then you won't need to assert the content of fileText (line 151)

Yes I was getting error with the [object object] and when I searched they recommended to use window.location. As I didn't want to change the component. Thanks I will do it

smartinellibenedetti · 2024-05-06T19:41:51Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    (editProjectFileService.getFileText as jest.Mock).mockImplementationOnce(
+      () => Promise.reject(new Error("Failed to fetch file")),
+    );
+    wrapper.vm.notifyError = jest.fn();


no need to override wrapper.vm.notifyError, you can use spyOn instead (https://jestjs.io/docs/jest-object#jestspyonobject-methodname)

smartinellibenedetti · 2024-05-06T19:42:04Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    (editProjectFileService.saveProjectFile as jest.Mock).mockRejectedValue(
+      new Error("Failed to save file"),
+    );
+    wrapper.vm.notifyError = jest.fn();


Same thing here, please use spyOn

smartinellibenedetti · 2024-05-06T19:48:23Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+  it("does not allow non-admin user to save the file", async () => {
+    await mountEditProjectFile({ authAdmin: false });
+    wrapper.vm.save = jest.fn();
+    wrapper.vm.getFileText = jest.fn();
+
+    wrapper.find('[data-test-id="save"]').trigger("click");
+    await wrapper.vm.$nextTick();
+    expect(wrapper.vm.save).not.toHaveBeenCalled();
+  });


This test is wrong, there's no 'save' method, which is the reason why after the click, it wasn't called.

But checked now the component logic and the API, a non-admin user will be able to save the file, but they won't be able to adjust where the message will be displayed. So safe to delete this test.

smartinellibenedetti · 2024-05-06T19:55:24Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    ).toHaveBeenCalledWith("default", "readme.md", "new content");
+  });
+
+  it("displays admin specific message and configuration link when user is an admin", async () => {


Suggested change

it("displays admin specific message and configuration link when user is an admin", async () => {

it("displays warning message and configuration link when user is an admin and displayConfig value is 'none'", async () => {

smartinellibenedetti · 2024-05-06T19:55:55Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    );
+  });
+
+  it("displays non-admin specific message when user is not an admin", async () => {


Suggested change

it("displays non-admin specific message when user is not an admin", async () => {

it("displays warning message and configuration link when user isn't an admin and displayConfig value is 'none'", async () => {

smartinellibenedetti · 2024-05-06T19:57:01Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+  it("renders the correct strings when user doesn't have permissions", async () => {
+    await mountEditProjectFile({ authAdmin: false });
+    expect(wrapper.text()).toContain(
+      "file.warning.not.displayed.nonadmin.message",
+    );
+  });
+  it("renders the correct strings when user has permissions", async () => {
+    expect(wrapper.text()).toContain(
+      "file.warning.not.displayed.admin.message",
+    );
+  });


repeated tests, please remove

smartinellibenedetti · 2024-05-06T19:57:25Z

...eckapp/grails-spa/packages/ui-trellis/src/app/components/readme-motd/EditProjectFile.spec.ts

+    await mountEditProjectFile({
+      authAdmin: false,
+    });
+    expect(wrapper.text()).toContain(


please use a data-test-id instead of using wrapper.text()

smartinellibenedetti

I'm marking this PR as approved as the tests are done, but I will also add a ticket to the backlog to refactor this test suite and clean it up.

It's fine to have the wrapper initialized before the tests. Whenever following this approach, it is required to be aware that a lot of actions may have already happened, which you don't need to re-trigger to be able to test things, while some you might need to trigger again, which may deviate from the component's original behavior if this component doesn't have any logic to do so.

jayas006 · 2024-05-08T19:22:35Z

I'm marking this PR as approved as the tests are done, but I will also add a ticket to the backlog to refactor this test suite and clean it up.

It's fine to have the wrapper initialized before the tests. Whenever following this approach, it is required to be aware that a lot of actions may have already happened, which you don't need to re-trigger to be able to test things, while some you might need to trigger again, which may deviate from the component's original behavior if this component doesn't have any logic to do so.

Yes, that makes sense.

jayas006 added 2 commits April 25, 2024 16:02

unit tests for edit project file

e39d1da

changed describe.each to it.each

daaeff5

jayas006 requested a review from smartinellibenedetti April 29, 2024 17:07

jayas006 commented Apr 29, 2024

View reviewed changes

smartinellibenedetti reviewed Apr 29, 2024

View reviewed changes

jayas006 added 5 commits April 29, 2024 18:30

removed un-necessary script

b2aa351

refactored tests

d57d5b4

reset file text on cancel

7662d52

add test for revert behaviour on cancel

71be35b

removed the unwanted script, updated tests

fd2910e

smartinellibenedetti requested changes May 6, 2024

View reviewed changes

jayas006 added 4 commits May 7, 2024 21:46

updated test

270021e

updated editprojectfile test

3b6970d

cleaned EditProjectFile test

36984ad

Update AceEditor test to check span HTML content

972c418

ltamaster added this to the 5.3.0 milestone May 8, 2024

ltamaster changed the title ~~Run- 2307 unit tests for edit project file~~ RUN-2307 unit tests for edit project file May 8, 2024

jayas006 added 2 commits May 8, 2024 12:51

added ref in ace-editor

eddb439

added jest.spyOn

8e3da89

smartinellibenedetti approved these changes May 8, 2024

View reviewed changes

ltamaster merged commit 1e2978e into main May 8, 2024
3 checks passed

ltamaster deleted the feat/unit-test-for-edit-project-file branch May 8, 2024 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RUN-2307 unit tests for edit project file #9084

RUN-2307 unit tests for edit project file #9084

jayas006 commented Apr 25, 2024 •

edited

jayas006 Apr 29, 2024

smartinellibenedetti Apr 29, 2024

smartinellibenedetti Apr 29, 2024

smartinellibenedetti left a comment

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

jayas006 May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti May 6, 2024

smartinellibenedetti left a comment

jayas006 commented May 8, 2024

	it("displays admin specific message and configuration link when user is an admin", async () => {
	it("displays warning message and configuration link when user is an admin and displayConfig value is 'none'", async () => {

	it("displays non-admin specific message when user is not an admin", async () => {
	it("displays warning message and configuration link when user isn't an admin and displayConfig value is 'none'", async () => {

RUN-2307 unit tests for edit project file #9084

RUN-2307 unit tests for edit project file #9084

Conversation

jayas006 commented Apr 25, 2024 • edited

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

smartinellibenedetti left a comment

Choose a reason for hiding this comment

jayas006 commented May 8, 2024

jayas006 commented Apr 25, 2024 •

edited