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

Learning analytics: Improve MetricsIntegrationTests #9306

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

raffifasaro
Copy link

@raffifasaro raffifasaro commented Sep 11, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I documented the Java code using JavaDoc style.

Motivation and Context

The MetricsIntegrationTests only had a very low coverage, and one of the existing tests was not working and deactivated. This PR should extend the test coverage to all metrics besides teamID.

Description

Fixed the test for AverageScore and reactivated it
Added tests for the following metrics parameters:

  • Categories
  • Score
  • Completed

Added tests for CompetencyInformation and LectureUnitInformation

The tests mainly focus on comparing the result from the "/api/metrics/course/courseID/student" request with test data pulled directly from the specific repositories. This also covers parts of the learningMetricsService, which is used by the endpoint to fetch the data.

Steps for Testing

  • Run the tests locally
  • Check that the test does not fail in the test system

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.







Review Progress

Performance Review

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced static methods in CompetencyInformationDTO and LectureUnitInformationDTO for streamlined object creation from existing entities.
    • Added a new method in StudentScoreUtilService to create student scores that include rated scores.
  • Bug Fixes

    • Adjusted coverage requirements for code testing to enforce stricter quality standards.
  • Tests

    • Expanded the MetricsIntegrationTest suite with new methods to validate various student metrics and competencies.
    • Introduced ExerciseTestRepository for enhanced data retrieval capabilities in testing scenarios.

+reduced jacoco missed classes threshold by one
+shouldReturnCompetencyInformation() test (WIP)
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) labels Sep 11, 2024
@raffifasaro raffifasaro changed the title Chore/learning analytics/improve metrics integration tests Learning analytics: Improve MetricsIntegrationTests Sep 11, 2024
@raffifasaro raffifasaro linked an issue Sep 11, 2024 that may be closed by this pull request
3 tasks
+ shouldReturnCompleted() test
+ shouldReturnScore() test
+ shouldReturnLectureUnitInformation() test (WIP)

+ use of lectureMetricsService (WIP)
…lytics/improve-metrics-integration-tests

# Conflicts:
#	src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/CompetencyInformationDTO.java
#	src/test/java/de/tum/cit/aet/artemis/AbstractArtemisIntegrationTest.java
#	src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java
some minor fixes
Removed ShouldReturnTeamId test
@raffifasaro raffifasaro marked this pull request as ready for review September 16, 2024 16:41
@raffifasaro raffifasaro requested a review from a team as a code owner September 16, 2024 16:41
Copy link

coderabbitai bot commented Sep 16, 2024

Walkthrough

This pull request introduces several changes, including a modification to the jacocoTestCoverageVerification task in the build.gradle file, which tightens the coverage requirements by lowering the maximum allowed missed count from 40 to 39. Additionally, new static factory methods are added to the CompetencyInformationDTO and LectureUnitInformationDTO classes for easier instantiation from their respective domain objects. The MetricsIntegrationTest class is expanded with new test methods and nested classes to enhance the testing of student metrics. A new method for creating rated student scores is also introduced in the StudentScoreUtilService, along with the addition of an ExerciseTestRepository interface for testing purposes.

Changes

File Change Summary
build.gradle Modified maximum in jacocoTestCoverageVerification from 40 to 39.
src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/CompetencyInformationDTO.java Added static method public static <C extends Competency> CompetencyInformationDTO of(C competency).
src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/LectureUnitInformationDTO.java Added static method public static <L extends LectureUnit> LectureUnitInformationDTO of(L lectureUnit).
src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java Expanded test class with new methods for verifying metrics and added nested classes for organization.
src/test/java/de/tum/cit/aet/artemis/StudentScoreUtilService.java Added method public void createStudentScoreIsRated(Exercise exercise, User user, double score).
src/test/java/de/tum/cit/aet/artemis/exercise/repository/ExerciseTestRepository.java Introduced ExerciseTestRepository interface with methods for fetching exercises with eager loading.

Possibly related issues

Possibly related PRs

Suggested labels

chore, ready to merge, template


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    -- I pushed a fix in commit <commit_id>, please review it.
    -- Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    -- @coderabbitai generate unit testing code for this file.
    -- @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    -- @coderabbitai generate interesting stats about this repository and render them as a table.
    -- @coderabbitai read src/utils.ts and generate unit testing code.
    -- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    -- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.
Early access features: enabled

We are currently testing the following features in early access:

  • OpenAI o1 for code reviews: OpenAI's new o1 model is being tested for generating code suggestions in code reviews.

Note:

  • You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 8ff8197 and 557ba36.

Files selected for processing (6)
  • build.gradle (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/CompetencyInformationDTO.java (2 hunks)
  • src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/LectureUnitInformationDTO.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java (5 hunks)
  • src/test/java/de/tum/cit/aet/artemis/StudentScoreUtilService.java (1 hunks)
  • src/test/java/de/tum/cit/aet/artemis/exercise/repository/ExerciseTestRepository.java (1 hunks)
Additional context used
Path-based instructions (5)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/LectureUnitInformationDTO.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/exercise/repository/ExerciseTestRepository.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/CompetencyInformationDTO.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/test/java/de/tum/cit/aet/artemis/StudentScoreUtilService.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

Additional comments not posted (16)
src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/LectureUnitInformationDTO.java (1)

21-31: LGTM!

The new static factory method of in the LectureUnitInformationDTO record is a great addition. It provides a clear and concise way to create DTOs from LectureUnit instances, promoting code reuse and reducing boilerplate. The method follows the single responsibility principle, uses constructor injection, and adheres to the KISS principle. It also aligns with the guidelines for DTOs by including minimal data and using the ZonedDateTime type for the releaseDate field. The method follows the Java naming convention as well. Overall, this is a well-designed and useful addition to the codebase.

src/main/java/de/tum/cit/aet/artemis/atlas/dto/metrics/CompetencyInformationDTO.java (1)

23-33: LGTM! The new static factory method enhances the functionality and promotes code reuse.

The new static factory method of in the CompetencyInformationDTO class is a great addition that provides several benefits:

  • It follows the single responsibility principle by focusing solely on creating a DTO from a domain object.
  • It promotes code reuse by providing a centralized way to convert a Competency to its DTO representation.
  • It uses constructor injection to create the DTO, aligning with the provided instructions.
  • It keeps the code simple and easy to understand, adhering to the KISS principle.
  • The method name of follows the Java naming convention for static factory methods.
  • The method parameters are effectively final, aligning with the least access principle.

Overall, this method enhances the functionality of the CompetencyInformationDTO class, promotes better encapsulation, and reduces boilerplate code in client classes that need to perform this conversion. Great job!

src/test/java/de/tum/cit/aet/artemis/StudentScoreUtilService.java (1)

37-52: LGTM!

The createStudentScoreIsRated method is well-implemented and follows the coding guidelines. It enhances the functionality of the StudentScoreUtilService by allowing the creation of scores that explicitly denote rated scores, providing a clearer distinction in score handling for exercises.

The method is small, focused, and follows a clear logical flow. It calculates the lastPoints based on the exercise's maximum points and the provided score percentage, ensuring consistency. The method also uses the studentScoreRepository to save the score, avoiding direct database access and promoting separation of concerns.

Great job!

src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java (12)

52-52: LGTM!

The addition of CompetencyRepository is necessary for testing competency-related functionality.


55-55: LGTM!

The addition of ExerciseTestRepository is necessary for testing exercise-related functionality.


58-58: LGTM!

The addition of LectureUnitRepository is necessary for testing lecture unit-related functionality.


61-61: LGTM!

The addition of StudentScoreRepository is necessary for testing student score-related functionality.


79-79: LGTM!

The addition of userID is necessary for testing user-specific functionality.


Line range hint 84-97: LGTM!

The changes to the setupTestScenario method are necessary for setting up the test data for the new test methods. The addition of the user with ID userID is required for testing user-specific functionality.


135-145: LGTM!

The shouldReturnCategories test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct categories for the exercises in the course.


147-163: LGTM!

The shouldReturnAverageScores test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct average scores for the exercises in the course.


165-182: LGTM!

The shouldReturnScore test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct scores for the exercises in the course.


266-282: LGTM!

The shouldReturnCompleted test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct completed exercises for the course.


288-304: LGTM!

The shouldReturnCompetencyInformation test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct competency information for the course.


310-331: LGTM!

The shouldReturnLectureUnitInformation test method is well-structured and follows the AAA pattern. The assertions are correct and verify that the /api/metrics/course/{courseId}/student endpoint returns the correct lecture unit information for the course.

build.gradle (1)

189-189: Looks good! This change aligns with the goal of improving test coverage.

Reducing the maximum threshold for MISSEDCOUNT is a positive step towards enhancing the codebase quality by enforcing stricter coverage requirements. Keep up the good work!

Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

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

Overall Looks Good to me

az108
az108 previously approved these changes Sep 17, 2024
Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

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

Code LGTM after clarifications

Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

Code mostly looks good. I have one small remark.

coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 17, 2024
@raffifasaro raffifasaro dismissed stale reviews from coderabbitai and az108 via 8efbfd0 September 17, 2024 13:01
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

Commits

Files that changed from the base of the PR and between 557ba36 and 8efbfd0.

Files selected for processing (1)
  • src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java (5 hunks)
Additional context used
Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/MetricsIntegrationTest.java (1)

Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true

Copy link
Contributor

@Jan-Thurner Jan-Thurner left a comment

Choose a reason for hiding this comment

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

code LGTM

Copy link
Contributor

@az108 az108 left a comment

Choose a reason for hiding this comment

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

reapprove after removing ;

@raffifasaro raffifasaro removed the request for review from JohannesStoehr September 17, 2024 14:38
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.

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

Learning analytics: Improve MetricsIntegrationTests
5 participants