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

Development: Refactor quiz server tests #6570

Merged
merged 39 commits into from
Jun 11, 2023

Conversation

laadvo
Copy link
Contributor

@laadvo laadvo commented May 12, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines
  • I documented the Java code using JavaDoc style.

Motivation and Context

There are many tests that do not follow the best testing practices and could be improved. There is a lot of duplicated code, overly long and unclear tests. Some assertions are missing and others are unnecessary. The PR tackels the before mentioned issues found in the QuizExerciseIntegrationTest.java class.

Description

Duplicated code has been reduced by introducing new helper functions which get reused in many tests. New and improved assertions have been in a few places and a two tests or so have been removed. Additionally, I also tried removing repository access wherever possible.

Note: there are four tests related to quiz statistics (testRecalculateStatistics, testReevaluateStatistics, testReevaluateStatistics_Practice and testReEvaluateQuizQuestionWithMoreSolutions) that I skipped. They are longer and more difficult to understand compared to other tests in the class, so I would prefer to deal with them at a later point.

Review Progress

Look through the code and make sure the refactorings make sense. The tests should be easier to understand and more efficient (less repository access).

Code Review

  • Code Review 1
  • Code Review 2

Test Coverage

unchanged

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

Merging #6570 (4bc8cbd) into develop (23f27a4) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6570      +/-   ##
=============================================
- Coverage      80.43%   80.41%   -0.02%     
  Complexity     13639    13639              
=============================================
  Files           2410     2410              
  Lines          91880    91880              
  Branches       12886    12886              
=============================================
- Hits           73900    73883      -17     
- Misses          9881     9898      +17     
  Partials        8099     8099              

see 12 files with indirect coverage changes

Flag Coverage Δ
client 76.14% <ø> (ø)
server 85.07% <ø> (-0.04%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23f27a4...4bc8cbd. Read the comment docs.

JohannesStoehr
JohannesStoehr previously approved these changes May 30, 2023
Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Code looks good after other reviews and merges of develop branch

DominikRemo
DominikRemo previously approved these changes May 30, 2023
Copy link
Contributor

@DominikRemo DominikRemo left a comment

Choose a reason for hiding this comment

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

Reapproved

DominikRemo
DominikRemo previously approved these changes May 31, 2023
Copy link
Contributor

@DominikRemo DominikRemo left a comment

Choose a reason for hiding this comment

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

Reapproved

julian-christl
julian-christl previously approved these changes May 31, 2023
Copy link
Member

@julian-christl julian-christl left a comment

Choose a reason for hiding this comment

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

Code approved, thanks.

Copy link
Member

@julian-christl julian-christl left a comment

Choose a reason for hiding this comment

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

Reapproved

Copy link
Contributor

@DominikRemo DominikRemo left a comment

Choose a reason for hiding this comment

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

Reapproved

@laadvo laadvo added this to the 6.2.2 milestone Jun 6, 2023
@krusche krusche merged commit 56a3bd3 into develop Jun 11, 2023
12 of 18 checks passed
@krusche krusche deleted the test/refactor-quiz-server-tests branch June 11, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Artemis Development
  
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants