Skip to content

test(frontend): extend GmailService spec to cover all methods#5460

Open
mengw15 wants to merge 1 commit into
apache:mainfrom
mengw15:test-5456-gmail-spec-extend
Open

test(frontend): extend GmailService spec to cover all methods#5460
mengw15 wants to merge 1 commit into
apache:mainfrom
mengw15:test-5456-gmail-spec-extend

Conversation

@mengw15

@mengw15 mengw15 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Extends the existing gmail.service.spec.ts (added in #5164, which covered sendEmail's success/error toasts) to cover the remaining surface of the 3-method service:

  • sendEmail request-body shape — explicit receiver, and the empty-string default when omitted
  • sendEmail error branch also logs console.error("Send email error:", …)
  • getSenderEmail() — a GET to /gmail/sender/email (text) that emits the body with no NotificationService side-effect
  • notifyUnauthorizedLogin — POST body shape, success toast, and error toast + console.error logging

Follows frontend/TESTING.md (Vitest, HttpClientTestingModule).

Any related issues, documentation, discussions?

Closes #5456. Builds on #5164.

How was this PR tested?

yarn test --include='**/gmail.service.spec.ts' → 9 passed. prettier --check clean.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (claude-opus-4-7)

@github-actions github-actions Bot added the frontend Changes related to the frontend GUI label Jun 7, 2026
@codecov-commenter

codecov-commenter commented Jun 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.93%. Comparing base (65ce1e5) to head (99a4282).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5460      +/-   ##
============================================
- Coverage     51.93%   51.93%   -0.01%     
  Complexity     2472     2472              
============================================
  Files          1067     1068       +1     
  Lines         41258    41263       +5     
  Branches       4437     4438       +1     
============================================
+ Hits          21427    21429       +2     
- Misses        18571    18576       +5     
+ Partials       1260     1258       -2     
Flag Coverage Δ *Carryforward flag
access-control-service 42.22% <ø> (ø) Carriedforward from 65ce1e5
agent-service 33.76% <ø> (ø) Carriedforward from 65ce1e5
amber 52.91% <ø> (ø) Carriedforward from 65ce1e5
computing-unit-managing-service 1.65% <ø> (ø) Carriedforward from 65ce1e5
config-service 56.06% <ø> (ø) Carriedforward from 65ce1e5
file-service 38.32% <ø> (ø) Carriedforward from 65ce1e5
frontend 46.45% <ø> (+<0.01%) ⬆️
pyamber 90.69% <ø> (ø) Carriedforward from 65ce1e5
python 90.83% <ø> (ø) Carriedforward from 65ce1e5
workflow-compiling-service 58.69% <ø> (ø) Carriedforward from 65ce1e5

*This pull request uses carry forward flags. Click here to find out more.

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

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines +76 to +82
it("defaults the receiver to an empty string when it is omitted", () => {
service.sendEmail("subj", "body");

const req = httpTestingController.expectOne(r => r.url.endsWith("/gmail/send") && r.method === "PUT");
expect(req.request.body).toEqual({ receiver: "", subject: "subj", content: "body" });
req.flush(null);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this a bug or intentional? if the email receiver is empty, can an email be sent?

if it is a bug, please create an issue to follow up on fixing it. we can pin test as it is first, then in the fix, we can see it updates the test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not a bug — likely a vestigial default. If receiver is empty the backend sends to the requester's own address (if (receiver.isEmpty) user.getEmail), so nothing breaks. Both callers pass an explicit receiver anyway (share-access only after email validation; the admin form requires the email field), so the empty default isn't exercised in normal use. Agreed on pinning the test as-is; we can open a low-priority follow-up to clean up the unused default.

Comment on lines +94 to +106
it("issues a GET to the sender-email endpoint and emits the response without notifying", () => {
let emitted: string | undefined;
service.getSenderEmail().subscribe(value => (emitted = value as string));

const req = httpTestingController.expectOne(
r => r.url.endsWith("/gmail/sender/email") && r.method === "GET" && r.responseType === "text"
);
req.flush("sender@example.com");

expect(emitted).toBe("sender@example.com");
expect(notificationSpy.success).not.toHaveBeenCalled();
expect(notificationSpy.error).not.toHaveBeenCalled();
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we notify user though?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

getSenderEmail is only used on the admin Gmail-config page (ngOnInit) to display the configured sender address — a read-only fetch on an admin-only screen, and the component already logs failures to the console. A toast there would mostly be page-load noise, so I don't think we need to notify the user. Keeping the service a thin, non-notifying GET (unlike sendEmail, which toasts) is intentional. I'd leave it as-is.

Comment on lines +84 to +92
it("logs the underlying error to the console on a failed send", () => {
const consoleSpy = vi.spyOn(console, "error").mockImplementation(() => {});
service.sendEmail("subj", "body", "to@example.com");

const req = httpTestingController.expectOne(r => r.url.endsWith("/gmail/send") && r.method === "PUT");
req.flush("boom", { status: 502, statusText: "Bad Gateway" });

expect(consoleSpy).toHaveBeenCalledWith("Send email error:", "boom");
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we notify user? (besides console log)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes — the same error handler shows a user-facing toast (notificationService.error("Failed to send email. Please try again or contact admin.")) in addition to the console log. The toast was asserted in a separate test; I've merged it into this one so the failure case asserts both the toast and the console log together (matching the notifyUnauthorizedLogin failure test).

@Yicong-Huang Yicong-Huang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

follow ups may go into issues. tests are good for the current behavior.

The existing spec (added in apache#5164) covered sendEmail's success and
error toasts. Add the remaining behavior: sendEmail request-body shape
(explicit and defaulted receiver) and its console.error logging,
getSenderEmail's GET request with no notification side-effect, and
notifyUnauthorizedLogin's POST body, success toast, and error
toast + console logging.

Closes apache#5456.
@mengw15 mengw15 force-pushed the test-5456-gmail-spec-extend branch from e9119ba to 99a4282 Compare June 9, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Changes related to the frontend GUI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add spec for gmail.service.ts

3 participants