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 atlas tests #9352

Merged
merged 61 commits into from
Oct 2, 2024
Merged

Conversation

MaximilianAnzinger
Copy link
Contributor

@MaximilianAnzinger MaximilianAnzinger commented Sep 21, 2024

Checklist

General

Server

Motivation and Context

Description

Introduces AbstractAtlasIntegrationTest and moves all @Autowire into the abstract test class.

Review Progress

Code Review

  • Code Review 1
  • Code Review 2

Test Coverage

unchanged

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced a new abstract class for integration tests, providing a shared setup for various test classes.
  • Bug Fixes

    • Removed unnecessary dependencies from the LearningPathIntegrationTest class, simplifying its setup and execution.
  • Refactor

    • Refactored LearningPathIntegrationTest to extend the new abstract class, enhancing consistency and reducing complexity.

…ory and prevent unambiguous bean exception
@MaximilianAnzinger MaximilianAnzinger added this to the 7.5.5 milestone Sep 29, 2024
@krusche krusche modified the milestones: 7.5.5, 7.5.6 Sep 29, 2024
JohannesStoehr
JohannesStoehr previously approved these changes Sep 29, 2024
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.

Approve refactored code

cremertim
cremertim previously approved these changes Oct 1, 2024
Copy link
Contributor

@cremertim cremertim left a comment

Choose a reason for hiding this comment

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

Refactored code lgtm

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: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between ccebd40 and 62a76c6.

📒 Files selected for processing (1)
  • src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.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

📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
Learnt from: julian-christl
PR: ls1intum/Artemis#8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-02-23T00:03:06.365Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)

53-53: Verify the impact of extending a different base class.

Extending AbstractAtlasIntegrationTest instead of AbstractSpringIntegrationIndependentTest may introduce changes in the test setup, lifecycle, and available utilities. Carefully review the differences between these base classes and ensure that the existing test methods are still valid and compatible with the new base class.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 1, 2024
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 62a76c6 and 28b9ce4.

📒 Files selected for processing (1)
  • src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.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 (1)
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (1)

1-159: Overall, the code changes look good

The introduction of AbstractAtlasIntegrationTest consolidates the @Autowired dependencies into a common abstract class for the Atlas integration tests. This promotes code reuse and simplifies the test classes that will extend this abstract class.

import de.tum.cit.aet.artemis.shared.base.AbstractSpringIntegrationIndependentTest;
import de.tum.cit.aet.artemis.text.util.TextExerciseUtilService;

public abstract class AbstractAtlasIntegrationTest extends AbstractSpringIntegrationIndependentTest {
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Add JavaDoc comment to the class definition

Consider adding a JavaDoc comment to the AbstractAtlasIntegrationTest class to describe its purpose and usage. This will enhance code readability and help other developers understand the intent of this abstract test class.

Copy link
Contributor

@JohannesWt JohannesWt left a comment

Choose a reason for hiding this comment

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

Code 👍

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.

Approve refactoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
atlas Pull requests that affect the corresponding module ready to merge tests
Projects
Status: Merged
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants