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: Complete the test suite for TriesOverview Component #11906

Merged
merged 6 commits into from
Mar 1, 2024

Conversation

EshaanAgg
Copy link
Contributor

@EshaanAgg EshaanAgg commented Feb 20, 2024

Summary

  • Completed the test suite for the TriesOverview Component with VTL
  • I refactored some of the logic of the component as well, as I felt that the same better described the intended functionality of the component rather than passing or checking for null values.
    I might be misunderstanding some functionality or missing out on some edge cases, so a second set of eyes would be helpful!

References

Parent Issue

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

Comment on lines 120 to 122
return this.pastTries.length
? this.pastTries.find(t => t.correct === this.maxQuestionsCorrect).time_spent
: null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the length of pastTries is not 0, then we are guaranteed to have a try object as the result of line 121 as this.maxQuestionsCorrect would be returning the score of the same (and thus we would always find it).

If not, then we can return the same as null.

Copy link
Member

Choose a reason for hiding this comment

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

@EshaanAgg sorry, I just noticed that If, for some reason, there is a corrupted object, and a try.correct is undefined, then Math.max will give NaN, and the .find will be null. It is better to add the null check, as before, in case anything can happen with the tries objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay sir, got that! Will revert these changes.

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.

Overall it looks pretty good! Just a small consideration

});

test('renders the best score as 0 when there are no past tries', () => {
renderComponent();
Copy link
Member

Choose a reason for hiding this comment

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

We can pass the pastTries explicitly as an empty array and the totalQuestions value.

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! Thanks for the review!

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we forgot to update the code here 😅 I meant to add something like:

renderComponent({ pastTries: [] });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! I totally missed this comment. Will update.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

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.

LGTM! Thanks @EshaanAgg!

@AlexVelezLl AlexVelezLl merged commit fd96c98 into learningequality:develop Mar 1, 2024
31 checks passed
@EshaanAgg EshaanAgg deleted the test1 branch April 7, 2024 08:51
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.

2 participants