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.

Sign up for GitHub

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: Remove deprecated router module in client tests #9439

Merged
merged 6 commits into from
Oct 8, 2024
Merged

Development: Remove deprecated router module in client tests #9439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bb7faa8
remove routerTestingModule part 1
JohannesWt Oct 7, 2024
e28ccaa
remove routerTestingModule part 2
JohannesWt Oct 8, 2024
e5d8554
fix student-exam-detail.component.spec.ts
JohannesWt Oct 8, 2024
81b451f
review feedback
JohannesWt Oct 8, 2024
e1cb2bb
I'm sorry Patrik. Now the empty imports are gone.
JohannesWt Oct 8, 2024
f4b46bf
move router to import
JohannesWt Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions ...cript/spec/component/assessment-dashboard/exercise-assessment-dashboard.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing';
import { ArtemisTestModule } from '../../test.module';
import { MockComponent, MockDirective, MockModule, MockPipe, MockProvider } from 'ng-mocks';
import { ActivatedRoute, Router, convertToParamMap } from '@angular/router';
import { ActivatedRoute, Router, RouterModule, convertToParamMap } from '@angular/router';
import { of, throwError } from 'rxjs';
import { HttpErrorResponse, HttpHeaders, HttpResponse } from '@angular/common/http';
import { SidePanelComponent } from 'app/shared/side-panel/side-panel.component';
Expand Down Expand Up @@ -57,7 +57,6 @@ import { MockTranslateValuesDirective } from '../../helpers/mocks/directive/mock
import { PieChartModule } from '@swimlane/ngx-charts';
import { LocalStorageService, SessionStorageService } from 'ngx-webstorage';
import { MockSyncStorage } from '../../helpers/mocks/service/mock-sync-storage.service';
import { RouterTestingModule } from '@angular/router/testing';
import { SortService } from 'app/shared/service/sort.service';
import { ArtemisMarkdownService } from 'app/shared/markdown.service';
import { AccountService } from 'app/core/auth/account.service';
Expand Down Expand Up @@ -211,7 +210,7 @@ describe('ExerciseAssessmentDashboardComponent', () => {
}),
},
} as any as ActivatedRoute;
const imports = [ArtemisTestModule, RouterTestingModule.withRoutes([]), MockModule(PieChartModule)];
const imports = [ArtemisTestModule, MockModule(PieChartModule), RouterModule.forRoot([])];

const declarations = [
ExerciseAssessmentDashboardComponent,
Expand Down
4 changes: 2 additions & 2 deletions src/test/javascript/spec/component/assessment-shared/assessment-header.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { AssessmentHeaderComponent } from 'app/assessment/assessment-header/asse
import { ArtemisTestModule } from '../../test.module';
import { Result } from 'app/entities/result.model';
import { AlertOverlayComponent } from 'app/shared/alert/alert-overlay.component';
import { RouterTestingModule } from '@angular/router/testing';
import { AssessmentWarningComponent } from 'app/assessment/assessment-warning/assessment-warning.component';
import { MockProvider } from 'ng-mocks';
import { Exercise, ExerciseType } from 'app/entities/exercise.model';
Expand All @@ -25,6 +24,7 @@ import { MockTranslateValuesDirective } from '../../helpers/mocks/directive/mock
import { NgbTooltipMocksModule } from '../../helpers/mocks/directive/ngbTooltipMocks.module';
import { NgbAlertsMocksModule } from '../../helpers/mocks/directive/ngbAlertsMocks.module';
import { AssessmentNote } from 'app/entities/assessment-note.model';
import { RouterModule } from '@angular/router';

describe('AssessmentHeaderComponent', () => {
let component: AssessmentHeaderComponent;
Expand All @@ -49,7 +49,7 @@ describe('AssessmentHeaderComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule, NgbTooltipMocksModule, NgbAlertsMocksModule],
imports: [ArtemisTestModule, NgbTooltipMocksModule, NgbAlertsMocksModule, RouterModule.forRoot([])],
declarations: [AssessmentHeaderComponent, AssessmentWarningComponent, AlertOverlayComponent, TranslateDirective, ArtemisTranslatePipe, MockTranslateValuesDirective],
providers: [
{
Expand Down
6 changes: 3 additions & 3 deletions ...spec/component/competencies/competency-management/competency-management.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,8 @@ import { MockComponent, MockDirective, MockPipe, MockProvider } from 'ng-mocks';
import { of } from 'rxjs';
import { Competency, CompetencyRelation, CompetencyWithTailRelationDTO, CourseCompetencyProgress, CourseCompetencyType } from 'app/entities/competency.model';
import { CompetencyManagementComponent } from 'app/course/competencies/competency-management/competency-management.component';
import { ActivatedRoute } from '@angular/router';
import { ActivatedRoute, provideRouter } from '@angular/router';
import { DeleteButtonDirective } from 'app/shared/delete-dialog/delete-button.directive';
import { RouterTestingModule } from '@angular/router/testing';
import { TextUnit } from 'app/entities/lecture-unit/textUnit.model';
import { AccountService } from 'app/core/auth/account.service';
import { ArtemisTestModule } from '../../../test.module';
Expand Down Expand Up @@ -47,7 +46,7 @@ describe('CompetencyManagementComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([]), NgbProgressbar],
imports: [ArtemisTestModule, NgbProgressbar],
declarations: [
CompetencyManagementComponent,
MockHasAnyAuthorityDirective,
Expand All @@ -61,6 +60,7 @@ describe('CompetencyManagementComponent', () => {
MockDirective(DeleteButtonDirective),
],
providers: [
provideRouter([]),
MockProvider(AccountService),
MockProvider(AlertService),
{ provide: NgbModal, useClass: MockNgbModalService },
Expand Down
4 changes: 2 additions & 2 deletions src/test/javascript/spec/component/competencies/competency-popover.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@ import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testin
import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe';
import { MockComponent, MockPipe } from 'ng-mocks';
import { FaIconComponent } from '@fortawesome/angular-fontawesome';
import { RouterTestingModule } from '@angular/router/testing';
import { CompetenciesPopoverComponent } from 'app/course/competencies/competencies-popover/competencies-popover.component';
import { By } from '@angular/platform-browser';
import { Component } from '@angular/core';
import { NgbPopoverModule } from '@ng-bootstrap/ng-bootstrap';
import { RouterModule } from '@angular/router';

@Component({
selector: 'jhi-statistics',
Expand All @@ -29,7 +29,7 @@ describe('CompetencyPopoverComponent', () => {
TestBed.configureTestingModule({
imports: [
NgbPopoverModule,
RouterTestingModule.withRoutes([
RouterModule.forRoot([
{ path: 'courses/:courseId/competencies', component: DummyStatisticsComponent },
{ path: 'course-management/:courseId/competency-management', component: DummyManagementComponent },
]),
Comment on lines +32 to 35
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: TestBed configuration updated correctly.

The replacement of RouterTestingModule with RouterModule.forRoot() is implemented correctly. This change provides a more realistic routing setup for the tests while maintaining the existing route structure.

Consider extracting the routes to a separate constant for better readability and reusability:

const testRoutes = [
  { path: 'courses/:courseId/competencies', component: DummyStatisticsComponent },
  { path: 'course-management/:courseId/competency-management', component: DummyManagementComponent },
];

// In TestBed configuration
RouterModule.forRoot(testRoutes),

This approach can improve maintainability if these routes are used in multiple test files.

Expand Down
6 changes: 3 additions & 3 deletions src/test/javascript/spec/component/complaints/complaints-for-tutor.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { ComplaintsForTutorComponent } from 'app/complaints/complaints-for-tutor
import { ExerciseGroup } from 'app/entities/exercise-group.model';
import { TextareaCounterComponent } from 'app/shared/textarea/textarea-counter.component';
import { MockComponent, MockDirective, MockPipe, MockProvider } from 'ng-mocks';
import { RouterTestingModule } from '@angular/router/testing';
import { AlertService } from 'app/core/util/alert.service';
import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
Expand All @@ -18,6 +17,7 @@ import { of } from 'rxjs';
import { Course } from 'app/entities/course.model';
import { Exercise } from 'app/entities/exercise.model';
import { TranslateDirective } from 'app/shared/language/translate.directive';
import { provideRouter } from '@angular/router';

describe('ComplaintsForTutorComponent', () => {
let complaintsForTutorComponent: ComplaintsForTutorComponent;
Expand All @@ -29,15 +29,15 @@ describe('ComplaintsForTutorComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [RouterTestingModule.withRoutes([]), FormsModule],
imports: [FormsModule],
declarations: [
ComplaintsForTutorComponent,
MockPipe(ArtemisTranslatePipe),
MockPipe(ArtemisDatePipe),
MockComponent(TextareaCounterComponent),
MockDirective(TranslateDirective),
],
providers: [MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService)],
providers: [provideRouter([]), MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService)],
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: TestBed configuration updated correctly

The replacement of RouterTestingModule with provideRouter([]) in the TestBed configuration is correct and aligns with Angular's latest best practices for testing components with routing dependencies.

Consider moving provideRouter([]) to the beginning of the providers array for better visibility:

-            providers: [MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService), provideRouter([])],
+            providers: [provideRouter([]), MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService)],
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
providers: [provideRouter([]), MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService)],
providers: [provideRouter([]), MockProvider(ComplaintResponseService), MockProvider(ComplaintService), MockProvider(AlertService)],

})
.compileComponents()
.then(() => {
Expand Down
5 changes: 2 additions & 3 deletions src/test/javascript/spec/component/course/course-exercises.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { of } from 'rxjs';
import { Course } from 'app/entities/course.model';
import { MockComponent, MockDirective, MockModule, MockPipe } from 'ng-mocks';
import { OrionFilterDirective } from 'app/shared/orion/orion-filter.directive';
import { RouterTestingModule } from '@angular/router/testing';
import { MockHasAnyAuthorityDirective } from '../../helpers/mocks/directive/mock-has-any-authority.directive';
import { TranslateService } from '@ngx-translate/core';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
Expand All @@ -15,7 +14,7 @@ import { CourseExercisesComponent } from 'app/overview/course-exercises/course-e
import { CourseExerciseRowComponent } from 'app/overview/course-exercises/course-exercise-row.component';
import { SidePanelComponent } from 'app/shared/side-panel/side-panel.component';
import { MockTranslateService, TranslatePipeMock } from '../../helpers/mocks/service/mock-translate.service';
import { ActivatedRoute } from '@angular/router';
import { ActivatedRoute, RouterModule } from '@angular/router';
import { ModelingExercise } from 'app/entities/modeling-exercise.model';
import { Exercise } from 'app/entities/exercise.model';
import dayjs from 'dayjs/esm';
Expand Down Expand Up @@ -45,7 +44,7 @@ describe('CourseExercisesComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, FormsModule, RouterTestingModule.withRoutes([]), MockModule(ReactiveFormsModule), MockDirective(TranslateDirective)],
imports: [ArtemisTestModule, FormsModule, RouterModule.forRoot([]), MockModule(ReactiveFormsModule), MockDirective(TranslateDirective)],
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: TestBed configuration updated correctly.

The TestBed configuration has been updated to use RouterModule.forRoot([]) instead of RouterTestingModule. This change aligns with the PR objective and Angular's latest best practices for component testing.

Consider extracting the router configuration to a constant for better readability:

const routes: Routes = [];
// ...
imports: [ArtemisTestModule, FormsModule, RouterModule.forRoot(routes), MockModule(ReactiveFormsModule), MockDirective(TranslateDirective)],

This approach improves code organization and makes it easier to add routes in the future if needed.

declarations: [
CourseExercisesComponent,
SidebarComponent,
Expand Down
5 changes: 3 additions & 2 deletions src/test/javascript/spec/component/course/course-management.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import { Course } from 'app/entities/course.model';
import { GuidedTourService } from 'app/guided-tour/guided-tour.service';
import { MockComponent, MockDirective, MockPipe, MockProvider } from 'ng-mocks';
import { OrionFilterDirective } from 'app/shared/orion/orion-filter.directive';
import { RouterTestingModule } from '@angular/router/testing';
import { MockHasAnyAuthorityDirective } from '../../helpers/mocks/directive/mock-has-any-authority.directive';
import { TranslateService } from '@ngx-translate/core';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
Expand All @@ -25,6 +24,7 @@ import { SortByDirective } from 'app/shared/sort/sort-by.directive';
import { SortDirective } from 'app/shared/sort/sort.directive';
import { DocumentationButtonComponent } from 'app/shared/components/documentation-button/documentation-button.component';
import { CourseAccessStorageService } from 'app/course/course-access-storage.service';
import { provideRouter } from '@angular/router';

describe('CourseManagementComponent', () => {
let fixture: ComponentFixture<CourseManagementComponent>;
Expand Down Expand Up @@ -90,7 +90,7 @@ describe('CourseManagementComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([])],
imports: [ArtemisTestModule],
declarations: [
CourseManagementComponent,
MockDirective(OrionFilterDirective),
Expand All @@ -104,6 +104,7 @@ describe('CourseManagementComponent', () => {
MockComponent(DocumentationButtonComponent),
],
providers: [
provideRouter([]),
{ provide: LocalStorageService, useClass: MockSyncStorage },
{ provide: SessionStorageService, useClass: MockSyncStorage },
MockProvider(TranslateService),
Expand Down
5 changes: 2 additions & 3 deletions src/test/javascript/spec/component/course/course-overview.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testin
import { CourseManagementService } from 'app/course/manage/course-management.service';
import { ArtemisTestModule } from '../../test.module';
import { HttpHeaders, HttpResponse } from '@angular/common/http';
import { ActivatedRoute, Params, Router } from '@angular/router';
import { ActivatedRoute, Params, Router, RouterModule } from '@angular/router';
import { Course, CourseInformationSharingConfiguration } from 'app/entities/course.model';
import { MockComponent, MockDirective, MockModule, MockPipe, MockProvider } from 'ng-mocks';
import { RouterTestingModule } from '@angular/router/testing';
import { MockHasAnyAuthorityDirective } from '../../helpers/mocks/directive/mock-has-any-authority.directive';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
import { CourseExerciseRowComponent } from 'app/overview/course-exercises/course-exercise-row.component';
Expand Down Expand Up @@ -165,8 +164,8 @@ describe('CourseOverviewComponent', () => {

TestBed.configureTestingModule({
imports: [
RouterModule.forRoot([]),
JohannesWt marked this conversation as resolved.
Show resolved Hide resolved
ArtemisTestModule,
RouterTestingModule.withRoutes([]),
MockModule(MatSidenavModule),
MockModule(NgbTooltipModule),
MockModule(BrowserAnimationsModule),
Expand Down
4 changes: 2 additions & 2 deletions ...pt/spec/component/course/course-statistics/course-management-statistics.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { ArtemisTestModule } from '../../../test.module';
import { MockSyncStorage } from '../../../helpers/mocks/service/mock-sync-storage.service';
import { LocalStorageService, SessionStorageService } from 'ngx-webstorage';
import { MockComponent, MockDirective, MockPipe } from 'ng-mocks';
import { RouterTestingModule } from '@angular/router/testing';
import { MockHasAnyAuthorityDirective } from '../../../helpers/mocks/directive/mock-has-any-authority.directive';
import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
Expand All @@ -15,6 +14,7 @@ import { of } from 'rxjs';
import { StatisticsAverageScoreGraphComponent } from 'app/shared/statistics-graph/statistics-average-score-graph.component';
import { ExerciseType } from 'app/entities/exercise.model';
import { DocumentationButtonComponent } from 'app/shared/components/documentation-button/documentation-button.component';
import { RouterModule } from '@angular/router';

describe('CourseManagementStatisticsComponent', () => {
let fixture: ComponentFixture<CourseManagementStatisticsComponent>;
Expand All @@ -31,7 +31,7 @@ describe('CourseManagementStatisticsComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([])],
imports: [ArtemisTestModule, RouterModule.forRoot([])],
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Approved: RouterTestingModule replacement implemented correctly.

The change from RouterTestingModule to RouterModule.forRoot([]) successfully addresses the deprecation issue mentioned in the PR objectives. This update aligns with Angular's latest best practices for testing components with routing dependencies.

However, consider the following suggestions for potential improvements:

  1. If the component under test relies on specific route configurations, you might want to add mock routes to forRoot([]). For example:

    RouterModule.forRoot([{ path: 'test', component: MockComponent }])

  2. Alternatively, you could use provideRouter([]) from @angular/router for a more streamlined setup, especially if you're using Angular 14 or later:

    import { provideRouter } from '@angular/router';
// ...
providers: [provideRouter([])]

These suggestions aim to ensure that the routing setup in the test environment closely mirrors the actual usage in the component.

declarations: [
CourseManagementStatisticsComponent,
MockComponent(StatisticsGraphComponent),
Expand Down
7 changes: 3 additions & 4 deletions ...ipt/spec/component/course/course-unenrollment/course-unenrollment-modal.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ import { By } from '@angular/platform-browser';
import { TranslateService } from '@ngx-translate/core';
import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe';
import { ReactiveFormsModule } from '@angular/forms';
import { RouterTestingModule } from '@angular/router/testing';
import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe';
import { Router } from '@angular/router';
import { Router, provideRouter } from '@angular/router';

describe('CourseRegistrationButtonComponent', () => {
let fixture: ComponentFixture<CourseUnenrollmentModalComponent>;
Expand All @@ -29,9 +28,9 @@ describe('CourseRegistrationButtonComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, ReactiveFormsModule, RouterTestingModule.withRoutes([])],
imports: [ArtemisTestModule, ReactiveFormsModule],
declarations: [CourseUnenrollmentModalComponent, MockPipe(ArtemisTranslatePipe), MockPipe(ArtemisDatePipe)],
providers: [MockProvider(CourseManagementService), MockProvider(AlertService), MockProvider(TranslateService)],
providers: [provideRouter([]), MockProvider(CourseManagementService), MockProvider(AlertService), MockProvider(TranslateService)],
Comment on lines +31 to +33
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: TestBed configuration updated correctly.

The TestBed configuration has been properly updated to use provideRouter([]) instead of RouterTestingModule. This change is consistent with the PR objectives and follows best practices for Angular testing.

A minor suggestion for consistency:

Consider moving provideRouter([]) to the end of the providers array to maintain the existing order of mock providers. This doesn't affect functionality but improves readability:

providers: [
  MockProvider(CourseManagementService),
  MockProvider(AlertService),
  MockProvider(TranslateService),
  provideRouter([])
],

})
.compileComponents()
.then(() => {
Expand Down
5 changes: 2 additions & 3 deletions src/test/javascript/spec/component/course/course.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { HttpHeaders, HttpResponse } from '@angular/common/http';
import { HttpTestingController } from '@angular/common/http/testing';
import { ComponentFixture, TestBed, fakeAsync, tick } from '@angular/core/testing';
import { ActivatedRoute, Router } from '@angular/router';
import { ActivatedRoute, Router, RouterModule } from '@angular/router';
import { Location } from '@angular/common';
import { RouterTestingModule } from '@angular/router/testing';
import { TranslateService } from '@ngx-translate/core';
import { DueDateStat } from 'app/course/dashboards/due-date-stat.model';
import { CourseForDashboardDTO } from 'app/course/manage/course-for-dashboard-dto';
Expand Down Expand Up @@ -93,7 +92,7 @@ describe('CoursesComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([{ path: 'courses/:courseId/exams/:examId', component: DummyComponent }])],
imports: [ArtemisTestModule, RouterModule.forRoot([{ path: 'courses/:courseId/exams/:examId', component: DummyComponent }])],
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using provideRouter() for a more lightweight test setup.

While the change from RouterTestingModule to RouterModule.forRoot() aligns with the PR objective, using RouterModule.forRoot() in tests sets up the full router configuration, which might be unnecessary for most test scenarios. Consider using provideRouter() for a more lightweight and focused test setup.

Here's a suggested refactor:

-imports: [ArtemisTestModule, RouterModule.forRoot([{ path: 'courses/:courseId/exams/:examId', component: DummyComponent }])],
+imports: [ArtemisTestModule],
+providers: [
+  ...
+  provideRouter([{ path: 'courses/:courseId/exams/:examId', component: DummyComponent }])
+],

This change will provide a more efficient routing setup for your tests while still allowing you to test navigation-related functionality.

Committable suggestion was skipped due to low confidence.

declarations: [
CoursesComponent,
MockDirective(MockHasAnyAuthorityDirective),
Expand Down
5 changes: 3 additions & 2 deletions ...st/javascript/spec/component/course/detail/course-detail-doughnut-chart.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { RouterTestingModule } from '@angular/router/testing';
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { ArtemisTestModule } from '../../../test.module';
import { CourseDetailDoughnutChartComponent } from 'app/course/manage/detail/course-detail-doughnut-chart.component';
Expand All @@ -7,6 +6,7 @@ import { MockModule, MockPipe } from 'ng-mocks';
import { DoughnutChartType } from 'app/course/manage/detail/course-detail.component';
import { Course } from 'app/entities/course.model';
import { PieChartModule } from '@swimlane/ngx-charts';
import { provideRouter } from '@angular/router';

describe('CourseDetailDoughnutChartComponent', () => {
let fixture: ComponentFixture<CourseDetailDoughnutChartComponent>;
Expand All @@ -19,8 +19,9 @@ describe('CourseDetailDoughnutChartComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([]), MockModule(PieChartModule)],
imports: [ArtemisTestModule, MockModule(PieChartModule)],
declarations: [CourseDetailDoughnutChartComponent, MockPipe(ArtemisTranslatePipe)],
providers: [provideRouter([])],
})
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: Addition of provideRouter to providers.

The addition of provideRouter([]) to the providers array is correct and aligns with the PR objective of replacing RouterTestingModule. This change follows Angular's latest best practices for setting up routing in test environments.

Consider adding a comment explaining why an empty array is used for provideRouter. This can help future developers understand that no specific routes are needed for these tests. For example:

providers: [
  // Provide empty routes array as no specific routes are needed for these tests
  provideRouter([])
],

.compileComponents()
.then(() => {
Expand Down
4 changes: 2 additions & 2 deletions ...script/spec/component/exam/exam-scores/exam-scores-average-scores-graph.component.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { ComponentFixture, TestBed } from '@angular/core/testing';
import { RouterTestingModule } from '@angular/router/testing';
import { TranslateService } from '@ngx-translate/core';
import { MockDirective, MockModule, MockPipe, MockProvider } from 'ng-mocks';
import { of } from 'rxjs';
Expand All @@ -16,6 +15,7 @@ import { NgxChartsSingleSeriesDataEntry } from 'app/shared/chart/ngx-charts-data
import { ExerciseType } from 'app/entities/exercise.model';
import { LocaleConversionService } from 'app/shared/service/locale-conversion.service';
import { TranslateDirective } from 'app/shared/language/translate.directive';
import { RouterModule } from '@angular/router';

describe('ExamScoresAverageScoresGraphComponent', () => {
let fixture: ComponentFixture<ExamScoresAverageScoresGraphComponent>;
Expand Down Expand Up @@ -55,7 +55,7 @@ describe('ExamScoresAverageScoresGraphComponent', () => {

beforeEach(() => {
TestBed.configureTestingModule({
imports: [ArtemisTestModule, RouterTestingModule.withRoutes([]), MockModule(BarChartModule)],
imports: [ArtemisTestModule, MockModule(BarChartModule), RouterModule.forRoot([])],
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM: TestBed configuration updated correctly.

The replacement of RouterTestingModule with RouterModule.forRoot([]) is correct and aligns with the PR objectives. This change follows Angular's latest best practices for setting up routing in tests.

Consider extracting the router configuration to a constant at the top of the file for better maintainability:

const routes = [];
// ... other setup code ...
imports: [ArtemisTestModule, MockModule(BarChartModule), RouterModule.forRoot(routes)],

This approach allows for easier modification of routes if needed in the future.

declarations: [ExamScoresAverageScoresGraphComponent, MockPipe(ArtemisTranslatePipe), MockDirective(TranslateDirective)],
providers: [
MockProvider(CourseManagementService, {
Expand Down
Loading
Loading