Skip to content
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-2307 unit tests for edit project file #9084

Merged
merged 13 commits into from
May 8, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
import { mount } from "@vue/test-utils";
import EditProjectFile from "./EditProjectFile.vue";
import * as editProjectFileService from "./editProjectFileService";

jest.mock("@/app/components/readme-motd/editProjectFileService", () => ({
getFileText: jest.fn().mockResolvedValue({
success: true,
contents: "sample file content",
}),

saveProjectFile: jest.fn().mockResolvedValue({
success: true,
message: "File saved successfully.",
}),
}));

jest.mock("@/library/rundeckService", () => ({
getRundeckContext: jest.fn().mockReturnValue({
rundeckClient: {
getFileText: jest.fn().mockResolvedValue({
filename: "readme.md",
success: true,
contents: "Some content",
rdBase: "http://localhost:4440",
}),
saveProjectFile: jest.fn().mockResolvedValue({
success: true,
message: "File saved successfully.",
}),
},
rdBase: "http://localhost:4440",
}),
url: jest.fn().mockReturnValue("http://localhost:4440"),
}));
let wrapper;
const mountEditProjectFile = async (props = {}) => {
wrapper = mount(EditProjectFile, {
props: {
filename: "readme.md",
project: "default",
authAdmin: true,
displayConfig: ["none"],
...props,
},
global: {
mocks: {
$t: (msg) => msg,
},
},
});
await wrapper.vm.$nextTick();
};

describe("EditProjectFile", () => {
beforeEach(async () => {
await mountEditProjectFile();
});

afterEach(() => {
jest.clearAllMocks();
});
it.each([
["readme.md", "edit.readme.label"],
["motd.md", "edit.motd.label"],
])("renders the correct title for %s", async (filename, expectedTitle) => {
await mountEditProjectFile({ filename });
expect(wrapper.find('[data-test-id="title"]').text()).toContain(
expectedTitle,
);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

it("handles failure when getFileText method fails", async () => {
(editProjectFileService.getFileText as jest.Mock).mockImplementationOnce(
() => Promise.reject(new Error("Failed to fetch file")),
);
wrapper.vm.notifyError = jest.fn();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await wrapper.vm.getFileText();
expect(wrapper.vm.notifyError).toHaveBeenCalledWith("Failed to fetch file");
});

it("handles failure when user edits the file and fails to save it", async () => {
(editProjectFileService.saveProjectFile as jest.Mock).mockRejectedValue(
new Error("Failed to save file"),
);
wrapper.vm.notifyError = jest.fn();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here, please use spyOn

wrapper.vm.fileText = "new content";
await wrapper.find('[data-test-id="save"]').trigger("click");
expect(wrapper.vm.notifyError).toHaveBeenCalledWith("Failed to save file");
});

it("handles success when user edits the file and saves it", async () => {
wrapper.vm.fileText = "new content";
await wrapper.find('[data-test-id="save"]').trigger("click");
expect(
editProjectFileService.saveProjectFile as jest.Mock,
).toHaveBeenCalledWith("default", "readme.md", "new content");
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () => {

const footerText = wrapper.find(".card-footer").text();
expect(footerText).toContain("file.warning.not.displayed.admin.message");
expect(wrapper.find(".card-footer a").text()).toBe(
"project.configuration.label",
);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () => {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"file.warning.not.displayed.nonadmin.message",
);
});
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();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
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",
);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated tests, please remove

it("reverts edits and triggers the correct action when the cancel button is clicked", async () => {
wrapper.vm.originalFileText = "sample file content";
wrapper.vm.fileText = "edited content";
wrapper.vm.createProjectHomeLink = jest.fn(() => {
wrapper.vm.fileText = wrapper.vm.originalFileText;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

await wrapper.find('[data-test-id="cancel"]').trigger("click");
expect(wrapper.vm.createProjectHomeLink).toHaveBeenCalled();
expect(wrapper.vm.fileText).toBe("sample file content");
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<div class="content">
<div id="layoutBody">
<div class="title">
<span class="text-h3">
<span class="text-h3" data-test-id="title">
<template v-if="filename === 'readme.md'">
<i class="fas fa-file-alt"></i>
{{ $t("edit.readme.label") }}
Expand Down Expand Up @@ -51,19 +51,22 @@
width="100%"
lang="markdown"
:read-only="false"
data-test-id="ace-editor"
/>
</div>
<div class="card-footer">
<button
type="button"
class="btn btn-default reset_page_confirm"
data-test-id="cancel"
@click="createProjectHomeLink"
>
Cancel
</button>
<button
type="submit"
class="btn btn-cta reset_page_confirm"
data-test-id="save"
@click="saveProjectFile"
>
Save
Expand Down Expand Up @@ -127,6 +130,7 @@ export default defineComponent({
data() {
return {
fileText: "",

markdownSectionOpen: false,
errorMsg: "",
};
Expand Down Expand Up @@ -185,6 +189,7 @@ export default defineComponent({
async getFileText() {
try {
const response = await getFileText(this.project, this.filename);

if (response.success) {
this.fileText = response.contents;
}
Expand Down