Skip to content

Add Makefile target coverage-report for CI-friendly coverage output (fixes #137) #142

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

Jump to bottom
Closed
Santosh69 wants to merge 2 commits into from
Closed

Add Makefile target coverage-report for CI-friendly coverage output (fixes #137) #142

Santosh69 wants to merge 2 commits into from
+74 −7

Conversation

@Santosh69
Copy link

@Santosh69 Santosh69 commented Nov 27, 2025 •
edited
Loading

Summary

This PR adds a new Makefile target coverage-report that prints a CI-friendly coverage summary directly to the terminal.

  • New target: coverage-report
  • Purpose: provide a concise coverage summary that CI tools or scripts can easily consume.
  • The existing coverage target (HTML output) is left unchanged.

What I changed

Added this target to Makefile:

coverage-report:
	@go test -coverprofile=coverage.out ./...
	@go tool cover -func=coverage.out | grep total | awk '{print "Coverage:", $$3}'

Updated .PHONY to include coverage-report.

Why

Issue #137 requests a terminal-friendly coverage output.
The current Makefile only generates an HTML report.
This new target provides a simple, script-friendly summary without replacing the existing workflow.

How I tested (local)

  • Ran make coverage-report → confirmed coverage output is printed for all packages.
  • Verified coverage.out is generated.
  • Ran make coverage → confirmed the HTML report is still produced successfully.

Screenshots

  1. Terminal output from make coverage-report:
    WhatsApp Image 2025-11-27 at 12 06 23 PM (2)

  2. Makefile change (coverage-report target):
    WhatsApp Image 2025-11-27 at 12 06 23 PM (1)

  3. HTML coverage report generated by make coverage:
    WhatsApp Image 2025-11-27 at 12 30 14 PM

Fixes #137

@CLAassistant
Copy link

CLAassistant commented Nov 27, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Santosh69
Copy link
Author

Santosh69 commented Nov 27, 2025

Hi @Ahmedhossamdev and @team 👋 ,

I’ve submitted this PR for issue #137.
When you get a chance, please take a look

If there are any follow-up improvements, related issues, or areas where this change can be expanded, I’d be happy to help.
I’m interested in contributing further to Maglev and open to taking on additional bugs or enhancements anytime.

@Santosh69 Santosh69 mentioned this pull request Nov 27, 2025
Open
aaronbrethorst
aaronbrethorst requested changes Nov 27, 2025
View reviewed changes
Copy link
Member

@aaronbrethorst aaronbrethorst left a comment

Choose a reason for hiding this comment

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

Please remove the unrelated AI generated changes from this PR and resubmit

internal/gtfs/direction_calculator.go
}
}

// Return the most common direction found in the directions map
Copy link
Member

@aaronbrethorst aaronbrethorst Nov 27, 2025

Choose a reason for hiding this comment

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

This comment doesn't add any value. It feels like it was generated by an overeager generative AI system.

internal/restapi/stops_for_location_handler.go
direction = stop.Direction.String
}
// Use the shared OBA direction pipeline (precomputed DB or shape-based fallback) for consistency.
direction := api.calculateStopDirection(ctx, stop.ID)
Copy link
Member

@aaronbrethorst aaronbrethorst Nov 27, 2025

Choose a reason for hiding this comment

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

This is not related to the state of purpose of your PR. Please revert and move it into a different commit

internal/restapi/stops_for_location_handler_test.go
valid := map[string]bool{
"N": true, "NE": true, "E": true, "SE": true,
"S": true, "SW": true, "W": true, "NW": true,
"UNKNOWN": true, // <--- correct fallback for THIS repo
Copy link
Member

@aaronbrethorst aaronbrethorst Nov 27, 2025

Choose a reason for hiding this comment

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

I would appreciate it if you'd remove obvious signs of these changes being generated by an AI agent

@aaronbrethorst
Copy link
Member

aaronbrethorst commented Jan 7, 2026

please review our policy on AI generated contributions and reopen when you're ready: https://opentransitsoftwarefoundation.org/2025/12/our-policy-on-ai-generated-contributions/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronbrethorst aaronbrethorst aaronbrethorst requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add Makefile Target for Test Coverage Report

3 participants

@Santosh69 @CLAassistant @aaronbrethorst