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

tests: add tests for some files in ContentRenderer #12056

Merged
merged 6 commits into from
Apr 18, 2024

Conversation

EshaanAgg
Copy link
Contributor

Summary

  • Added the tests for the components as well as utility JavaScript file in the ContentRenderer directory
  • The tests for the ContentRenderer/DownloadButton are not complete, as even when I pass valid files to the same, I can't see a dropdown being rendered in the DOM, and thus I am unable to test its user interaction. Would highly appreciate any help I can get on the same.

Reviewer guidance


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@MisRob MisRob added the TODO: needs review Waiting for review label Apr 9, 2024
@MisRob MisRob requested review from nucleogenesis, AllanOXDi, akolson and LianaHarris360 and removed request for LianaHarris360 April 9, 2024 04:34
@rtibbles
Copy link
Member

I am not sure - but I suspect that you are mocking isAppContext incorrectly. It is coming from the useUser composable: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/assets/src/views/ContentRenderer/DownloadButton.vue#L30 not from Vuex, so should be mocked using jest.mock('kolibri.coreVue.composables.useUser'); in your test file. For more details look at the useUser mock in the codebase on how to simulate specific values also.

@EshaanAgg
Copy link
Contributor Author

EshaanAgg commented Apr 12, 2024

My bad @rtibbles! Since the composable itself operates on a Vuex store, I though it would be better to mock the store directly. But I see the merit in mocking the composable directly in terms of the clarity and ease. Thanks for the feedback. I will update the same.

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

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

Thanks @EshaanAgg! The tests look correct to me. Furthermore, the isAppContext seems to be mocked correctly now.

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Hi @EshaanAgg! Thank you! This is looking fine! Just a couple of comments on some test names, and reminders that we dont need to include the import '@testing-library/jest-dom'; statement on every test suite 👐.

});

test('should not render if there are no downloadable files even if isAppContext is false', () => {
renderComponent({
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to find a better way of passing the useUserMock values, because reading just the testcase I could think that isAppContextis a prop of the component, but it isn't, so we can maybe wrap this value in a useUserMock object? This way the name suggest that we are passing this value to a mock of a composable. And we can end up in something like:

renderComponent({
  files: [],
  useUserMock: { isAppContext: false }
});

Let me know what do you think, as we can standarize this way of passing composables mock values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, this looks better! This provides a clear boundary between the props of the component, and all the other configuration that might be required by it, making it easier to reason about. Will update!

props: {
files: [],
nodeTitle: '',
...props,
Copy link
Member

Choose a reason for hiding this comment

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

Just flagging that we are still passing the isAppContext as prop here. To prevent this from happening, we can use destructuring:

const renderComponent = props => {
  const { isAppContext, ...componentProps } = props;
  ...
}; 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Since how we are using the name useUserMock for all the composable mocks, the structure would be a bit different, but I get the spirit. Thanks!

@EshaanAgg EshaanAgg requested a review from AlexVelezLl April 18, 2024 14:33
Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

Thanks @EshaanAgg! This looks great! LGTM.

@AlexVelezLl AlexVelezLl merged commit 7f7032a into learningequality:develop Apr 18, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants