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

fix: get launch url using exam id instead of attempt id #17

Merged
merged 1 commit into from
Nov 27, 2023

Conversation

varshamenon4
Copy link
Member

Instead of getting the launch url using the exam id, the ReviewExamAttemptButton was using the attempt id. This bug is fixed in this PR by getting the exam id using hooks. Ticket here: COSMO-63

@varshamenon4 varshamenon4 changed the title fix: change attempt id to exam id to get launch url for review attempt button fix: get launch url using exam id instead of attempt id in ReviewExamAttemptButton Nov 17, 2023
@varshamenon4 varshamenon4 changed the title fix: get launch url using exam id instead of attempt id in ReviewExamAttemptButton fix: get launch url using exam id instead of attempt id Nov 17, 2023
}));

describe('AttemptList', () => {
const defaultExamsData = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Because I'm now getting the currentExam via the useExamsData hook, I'm needing to mock this fn and create a default exams data in both AttemptList and the ReviewExamAttemptButton. I don't love that I need to have it in the AttemptList since it's just the parent component - wondering if there is a better way to mock this. I see a console error when these tests were failing about error boundaries: https://legacy.reactjs.org/docs/error-boundaries.html so I can look into that too if that's better.

Copy link
Member

Choose a reason for hiding this comment

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

You could try mocking out the actual children of AttemptList (e.g. ReviewExamAttemptButton)? That way, you're only testing the functionality in the AttemptList component without rendering the ReviewExamAttemptButton and needing to mock out the useExamsData hook.

However, I suspect that's not the testing strategy the React Testing Library would recommend, right? If I recall correctly, their approach is to test the application as closely as it would appear to the user. Mocking out children components is not that.

I was reading this thread from some ideas. It's a little old, though. But, based on the conversation in that thread, I wonder if the tests in ReviewExamAttemptButton.jsx should be moved into AttemptList.test.jsx, and the AttemptList.test.jsx acts as the E2E testing suite for that whole component?

I'm not sure what the right approach is here, but these are just some ideas. Maybe a good topic for standup?

Copy link
Member Author

@varshamenon4 varshamenon4 Nov 20, 2023

Choose a reason for hiding this comment

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

Notes:

  • In the learning assistant library, we set up a mock redux store. That could also solve the problem potentially. > is this recommended?
  • What we were using before worked well for mocking child components. React Testing Library kind of wants you to put together the whole testing state.
  • Approach is to test whole app, not in pieces.
  • ask in fedx channel about testing best practices
  • look at redux best practices (helper that sets up everything)
  • https://github.com/edx/frontend-lib-learning-assistant/blob/main/src/utils/utils.test.jsx#L10-L31

Copy link
Contributor

@zacharis278 zacharis278 Nov 20, 2023

Choose a reason for hiding this comment

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

maybe a simpler path than following the learning MFE, which handles everything (rendering, i18n, redux) is just to have a helper / setup code that populates a default state. That's pretty close to what we're already doing on the base component. Wouldn't require any large refactoring to make something like this a default for all tests: https://github.com/edx/frontend-app-exams-dashboard/blob/main/src/pages/ExamsPage/index.test.jsx#L26-L40

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool! Refactored to have a default for all tests in testutils.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3288715) 75.08% compared to head (022a865) 75.42%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #17      +/-   ##
==========================================
+ Coverage   75.08%   75.42%   +0.34%     
==========================================
  Files          22       22              
  Lines         289      293       +4     
  Branches       29       29              
==========================================
+ Hits          217      221       +4     
  Misses         69       69              
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@varshamenon4 varshamenon4 force-pushed the varshamenon4/fix-attempt-id-to-exam-id branch from 0c45eef to da4c11b Compare November 27, 2023 17:11
Copy link
Member Author

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

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

Refactored to have all of the default values live in a test utils file. But lemme know if there is a better place for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to have default state live here but let me know if there is a better place for this.

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

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

This looks great!

@varshamenon4 varshamenon4 merged commit 466c54a into main Nov 27, 2023
2 checks passed
@varshamenon4 varshamenon4 deleted the varshamenon4/fix-attempt-id-to-exam-id branch November 27, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants