-
Notifications
You must be signed in to change notification settings - Fork 900
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-2353-Refactor editProject test to improve readability and maintainability #9115
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.
there's still stuff to be cleaned up:
there are expects that are asserting exists,
there is an override of wrapper.vm.fileText that should be removed
there is a test that isn't using the spy anymore (or that didn't get added before)
Also, you shouldn't have the template hardcoding that string inside the span - you can change the template to render the content of the getValue method (you don't need to add a new var inside data for this)
I have tried to fix according to the review pointers. Please let me know if there are any further changes required or if any aspect of the test needs to be updated. |
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.
LGTM
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.
It's necessary to adjust the ace editor mock, otherwise it's not possible to assert that the component is rendering the results fetched from API. Please let me know in case of doubts.
const aceEditor = wrapper.findComponent({ name: "AceEditor" }); | ||
expect(aceEditor.exists()).toBe(true); | ||
aceEditor.vm.getValue(); |
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.
This line isn't needed.
aceEditor.vm.getValue(); |
jest.mock("../../../library/components/utils/AceEditor.vue", () => ({ | ||
name: "AceEditor", | ||
functional: true, | ||
template: '<span class="ace_text ace_xml"></span>', | ||
props: ["value"], | ||
template: '<span class="ace_text ace_xml">{{ getValue() }}</span>', | ||
methods: { | ||
getValue: jest.fn().mockReturnValue("sample file content"), | ||
}, | ||
})); |
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 are problems with this approach:
- this mock doesn't match any of the props that the component originally used, nor mock an existing method.
- furthermore, the way it is implemented it's not possible to override the value nor assert that the component is rendering the data retrieved from the API.
To do:
- change props to "modelValue" instead of "value";
- change template to render just "modelValue";
- remove getValue method;
…mplate to use modelValue, removed getValue method
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.
🚀 ship it, looking good!
Is this a bugfix, or an enhancement? Please describe.
This is an enhancement of the testing suite for the
EditProjectFile.vue
component.Describe the solution you've implemented
I've refactored the tests for
EditProjectFile.vue
to improve readability and maintainability. The changes include better organization of test cases, clearer descriptions, and removal of unnecessary global mocks.Describe alternatives you've considered
An alternative would have been to leave the tests as they were, but this would have made the test suite harder to maintain and understand in the long run.
Additional context