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

Clean up tests #3023

Merged
merged 2 commits into from
Feb 18, 2025
Merged

Clean up tests #3023

merged 2 commits into from
Feb 18, 2025

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Feb 18, 2025

I've removed unused things from module definitions.


This change is Reviewable

Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.65%. Comparing base (e59ddd3) to head (17838a1).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3023      +/-   ##
==========================================
+ Coverage   82.56%   82.65%   +0.08%     
==========================================
  Files         553      553              
  Lines       32006    32006              
  Branches     5164     5186      +22     
==========================================
+ Hits        26427    26455      +28     
+ Misses       4823     4782      -41     
- Partials      756      769      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Nateowami Nateowami force-pushed the feature/cleanup-tests branch from 79b4477 to 11261aa Compare February 18, 2025 01:03
@pmachapman pmachapman force-pushed the feature/cleanup-tests branch from 11261aa to 0a89512 Compare February 18, 2025 02:13
@pmachapman pmachapman self-requested a review February 18, 2025 02:13
@pmachapman pmachapman self-assigned this Feb 18, 2025
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

I am amazed how much code is removed and still the tests work?!?!?!

It seems a bunch of the mocks are still created, although no longer injected (I have marked the ones I could find).

Sorry for the repetitive nature of my comments. I tested the removals in a few of the files (but not all), so I think all the ones I flagged can be removed, but I imagine a couple perhaps will not be able to because the mock will be used as a return value for a function call or similar.

Reviewed 89 of 89 files at r1, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/checking/question-dialog/question-dialog.service.spec.ts line 38 at r1 (raw file):

const mockedProjectService = mock(SFProjectService);
const mockedQuestionsService = mock(CheckingQuestionsService);
const mockedUserService = mock(UserService);

Should this be removed?

Code quote:

const mockedFileService = mock(FileService);

src/SIL.XForge.Scripture/ClientApp/src/app/app.component.spec.ts line 68 at r1 (raw file):

const mockedUrlService = mock(ExternalUrlService);
const mockedFileService = mock(FileService);
const mockedErrorReportingService = mock(ErrorReportingService);

Should this be removed as well?

Code quote:

const mockedFeatureFlagService = mock(FeatureFlagService);

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-audio-player/checking-audio-player.component.spec.ts line 189 at r1 (raw file):

class TestEnvironment {
  readonly testOnlineStatusService: TestOnlineStatusService;
  readonly mockedI18nService = mock(I18nService);

Should this be removed?

Code quote:

readonly mockedI18nService = mock(I18nService);

src/SIL.XForge.Scripture/ClientApp/src/app/checking/chapter-audio-dialog/chapter-audio-dialog.component.spec.ts line 43 at r1 (raw file):

  ChapterAudioDialogComponent,
  ChapterAudioDialogData,
  ChapterAudioDialogResult

Should this be removed as well?

Code quote:

const mockedSFProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/connect-project/connect-project.component.spec.ts line 46 at r1 (raw file):

const mockedUserService = mock(UserService);
const mockedNoticeService = mock(NoticeService);
const mockedI18nService = mock(I18nService);

Should this be removed?

Code quote:

const mockedI18nService = mock(I18nService);

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/owner/owner.component.spec.ts line 95 at r1 (raw file):

  readonly fixture: ComponentFixture<HostComponent>;

  readonly mockedAuthService = mock(AuthService);

These should be removed.

Code quote:

  readonly mockedAuthService = mock(AuthService);
  readonly mockedBugsnagService = mock(BugsnagService);
  readonly mockedCookieService = mock(CookieService);

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/owner/owner.component.spec.ts line 97 at r1 (raw file):

  readonly mockedAuthService = mock(AuthService);
  readonly mockedBugsnagService = mock(BugsnagService);
  readonly mockedCookieService = mock(CookieService);

This one too.

Code quote:

readonly mockedUserService = mock(UserService);

src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-dialog.component.spec.ts line 33 at r1 (raw file):

import { ShareDialogComponent, ShareDialogData, ShareLinkType } from './share-dialog.component';

const mockedProjectService = mock(SFProjectService);

Should this be removed?

Code quote:

const mockedFeatureFlagService = mock(FeatureFlagService);

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.spec.ts line 28 at r1 (raw file):

import { UserService } from 'xforge-common/user.service';
import { ServalAdministrationService } from './serval-administration.service';
import { ServalProjectsComponent } from './serval-projects.component';

Should this be removed?

Code quote:

const mockedUserService = mock(UserService);

src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-button.component.spec.ts line 15 at r1 (raw file):

import { TestRealtimeService } from 'xforge-common/test-realtime.service';
import { configureTestingModule, TestTranslocoModule } from 'xforge-common/test-utils';
import { UICommonModule } from 'xforge-common/ui-common.module';

Should this be removed?

Code quote:

const mockedProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/text-chooser-dialog/text-chooser-dialog.component.spec.ts line 33 at r1 (raw file):

import { UserService } from 'xforge-common/user.service';
import { CheckingModule } from '../checking/checking.module';
import { SFProjectProfileDoc } from '../core/models/sf-project-profile-doc';

Should these be removed?

Code quote:

const mockedProjectService = mock(SFProjectService);
const mockedUserService = mock(UserService);

src/SIL.XForge.Scripture/ClientApp/src/app/project/project.component.spec.ts line 35 at r1 (raw file):

const mockedUserService = mock(UserService);
const mockedActivatedRoute = mock(ActivatedRoute);
const mockedRouter = mock(Router);

Should this be removed?

Code quote:

const mockedSFProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/project/project.component.spec.ts line 37 at r1 (raw file):

const mockedRouter = mock(Router);
const mockedSFProjectService = mock(SFProjectService);
const mockedTranslocoService = mock(TranslocoService);

Should this be removed, too?

Code quote:

const mockedPermissions = mock(PermissionsService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.spec.ts line 28 at r1 (raw file):

  let component: DraftGenerationStepsComponent;
  let fixture: ComponentFixture<DraftGenerationStepsComponent>;

Should this be removed?

Code quote:

 const mockActivatedRoute = mock(ActivatedRoute);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/training-progress/training-progress.component.spec.ts line 25 at r1 (raw file):

import { TrainingProgressComponent } from './training-progress.component';

const mockedProjectService = mock(SFProjectService);

Should this be removed?

Code quote:

const mockedProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/translate-metrics-session.spec.ts line 36 at r1 (raw file):

const mockedSFProjectService = mock(SFProjectService);
const mockedUserService = mock(UserService);

Should this be removed?

Code quote:

const mockedReportingService = mock(ErrorReportingService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts line 49 at r1 (raw file):

  let mockDraftGenerationService: jasmine.SpyObj<DraftGenerationService>;
  let mockDraftSourcesService: jasmine.SpyObj<DraftSourcesService>;
  let mockActivatedProjectService: jasmine.SpyObj<ActivatedProjectService>;

Should this be removed?

Code quote:

let mockI18nService: jasmine.SpyObj<I18nService>;

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts line 51 at r1 (raw file):

  let mockActivatedProjectService: jasmine.SpyObj<ActivatedProjectService>;
  let mockProjectService: jasmine.SpyObj<SFProjectService>;
  let mockUserService: jasmine.SpyObj<UserService>;

Should this be removed as well?

Code quote:

let mockPreTranslationSignupUrlService: jasmine.SpyObj<PreTranslationSignupUrlService>;

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.spec.ts line 61 at r1 (raw file):

const mockedDialogService = mock(DialogService);
const mockedNoticeService = mock(NoticeService);
const mockedProjectService = mock(SFProjectService);

Should this be removed?

Suggestion:

const mockedPermissions = mock(PermissionsService);

src/SIL.XForge.Scripture/ClientApp/src/app/users/collaborators/collaborators.component.spec.ts line 42 at r1 (raw file):

import { SharedModule } from '../../shared/shared.module';
import { paratextUsersFromRoles } from '../../shared/test-utils';
import { CollaboratorsComponent } from './collaborators.component';

Should this be removed?

Code quote:

const mockedLocationService = mock(LocationService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-history/history-chooser/history-chooser.component.spec.ts line 26 at r1 (raw file):

const mockedDialogService = mock(DialogService);
const mockedI18nService = mock(I18nService);

Should this be removed?

Code quote:

const mockedI18nService = mock(I18nService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts line 29 at r1 (raw file):

import { DraftHandlingService } from '../draft-handling.service';
import { DraftApplyDialogComponent } from './draft-apply-dialog.component';

Should this be removed?

Code quote:

const mockedDraftHandlingService = mock(DraftHandlingService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts line 35 at r1 (raw file):

const mockedUserService = mock(UserService);
const mockedDialogRef = mock(MatDialogRef);
const mockedTextDocService = mock(TextDocService);

Should this be removed too?

Code quote:

const mockedI18nService = mock(I18nService);

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio/audio-player-base/audio-player-base.component.spec.ts line 100 at r1 (raw file):

    expect(pause).toHaveBeenCalledTimes(1);
  }));
});

This should be removed.

Code quote:

readonly mockedProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/note-dialog/note-dialog.component.spec.ts line 59 at r1 (raw file):

import { SFProjectDoc } from '../../../core/models/sf-project-doc';
import { SFProjectProfileDoc } from '../../../core/models/sf-project-profile-doc';
import { SFProjectUserConfigDoc } from '../../../core/models/sf-project-user-config-doc';

Should this be removed?

Code quote:

const mockedProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/translate-overview/translate-overview.component.spec.ts line 45 at r1 (raw file):

import { TranslateOverviewComponent } from './translate-overview.component';

const mockedActivatedRoute = mock(ActivatedRoute);

Should this be removed?

Code quote:

const mockedSFProjectService = mock(SFProjectService);

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.spec.ts line 103 at r1 (raw file):

const mockedProjectService = mock(SFProjectService);
const mockedTranslationEngineService = mock(TranslationEngineService);
const mockedNoticeService = mock(NoticeService);

Should this be removed?

Code quote:

const mockedMediaBreakpointService = mock(MediaBreakpointService);

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.spec.ts line 105 at r1 (raw file):

const mockedNoticeService = mock(NoticeService);
const mockedActivatedRoute = mock(ActivatedRoute);
const mockedDialogService = mock(DialogService);

Should this be removed, too?

Code quote:

const mockedPermissions = mock(PermissionsService);

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor-history/editor-history.component.spec.ts line 26 at r1 (raw file):

  let fixture: ComponentFixture<EditorHistoryComponent>;
  const mockSFProjectService = mock(SFProjectService);
  const mockI18nService = mock(I18nService);

Should this be removed?

Code quote:

const mockEditorHistoryService = mock(EditorHistoryService);

@Nateowami Nateowami force-pushed the feature/cleanup-tests branch 2 times, most recently from 36f9937 to 2211d13 Compare February 18, 2025 16:02
Copy link
Collaborator

@pmachapman pmachapman 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 - just one you missed!

Reviewed 20 of 20 files at r2, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts line 29 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

@Nateowami You forgot this one.

Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Sorry; I didn't actually go through all your comments one-by-one. I wrote a migration that would remove most unused mocks, and it got most of what I missed. So most of the cleanup was in one fell swoop without following your individual comments.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @pmachapman)


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts line 29 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

@Nateowami You forgot this one.

There is a verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything())).never(); later in the file.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/translate-metrics-session.spec.ts line 36 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

It gets used in a bunch of verify statements.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/note-dialog/note-dialog.component.spec.ts line 59 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

It gets used here:

verify(mockedProjectService.createNoteThread(anything(), anything())).never();

@Nateowami Nateowami force-pushed the feature/cleanup-tests branch from 2211d13 to a045629 Compare February 18, 2025 19:44
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Nateowami)


src/SIL.XForge.Scripture/ClientApp/src/app/shared/share/share-button.component.spec.ts line 15 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

FIXME


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts line 49 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Should this be removed?

FIXME


src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.spec.ts line 29 at r1 (raw file):

Previously, Nateowami wrote…

There is a verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything())).never(); later in the file.

FIXME Yes, but the mockedDraftHandlingService isn't actually injected, and so it will always return never.


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/translate-metrics-session.spec.ts line 36 at r1 (raw file):

Previously, Nateowami wrote…

It gets used in a bunch of verify statements.

Sorry my bad!


src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/note-dialog/note-dialog.component.spec.ts line 59 at r1 (raw file):

Previously, Nateowami wrote…

It gets used here:

verify(mockedProjectService.createNoteThread(anything(), anything())).never();

FIXME

@Nateowami Nateowami force-pushed the feature/cleanup-tests branch from a045629 to 17838a1 Compare February 18, 2025 22:05
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

@pmachapman pmachapman enabled auto-merge (squash) February 18, 2025 22:12
@pmachapman pmachapman merged commit dcfa408 into master Feb 18, 2025
15 checks passed
@pmachapman pmachapman deleted the feature/cleanup-tests branch February 18, 2025 22:13
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