feat: add Makefile target to test sample projects with gfp test#172
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a new Makefile target to run Flow diagram for new test-gfp-projects Makefile targetflowchart TD
A[Developer runs
make test-gfp-projects] --> B[Makefile target
test-gfp-projects]
B --> C[cd ihp-gdsfactory--sample-projects/ihp--public--project]
C --> D[uv run --directory CURDIR gfp test]
D --> E[Sample project cells
tested via gfp]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider marking
test-gfp-projectsas a.PHONYtarget to avoid any ambiguity if a file or directory with that name appears in the repo. - If you expect to add more sample projects, it might be cleaner to define a variable containing the project paths and iterate over them in
test-gfp-projectsrather than hardcoding a singlecd.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider marking `test-gfp-projects` as a `.PHONY` target to avoid any ambiguity if a file or directory with that name appears in the repo.
- If you expect to add more sample projects, it might be cleaner to define a variable containing the project paths and iterate over them in `test-gfp-projects` rather than hardcoding a single `cd`.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request adds a new test-gfp-projects target to the Makefile to automate testing for sample projects. A high-severity issue was identified in the implementation where the --directory $(CURDIR) flag incorrectly forces the command to run in the root directory, negating the intended directory change. The reviewer provided suggestions to correctly target the sample project directory or use a loop to handle multiple projects.
| uv run pytest -s --force-regen | ||
|
|
||
| test-gfp-projects: | ||
| cd ihp-gdsfactory--sample-projects/ihp--public--project && uv run --directory $(CURDIR) gfp test |
There was a problem hiding this comment.
The current command incorrectly runs gfp test in the root directory instead of the sample project directory. The --directory $(CURDIR) flag instructs uv to change the working directory to the root of the repository (where the main Makefile is located) before executing the command, which negates the cd and causes gfp test to run on the main project.
To correctly test the sample project, you should point uv to the sample project's directory.
If you intend to support multiple sample projects in the future (as suggested by the target name and description), you might consider using a loop:
test-gfp-projects:
@for dir in ihp-gdsfactory--sample-projects/*/ ; do \
if [ -f "$$dir/pyproject.toml" ]; then \
uv run --directory $$dir gfp test; \
fi \
donetest-gfp-projects:
uv run --directory ihp-gdsfactory--sample-projects/ihp--public--project gfp test
Summary
test-gfp-projectsMakefile target that runsgfp testinside each sample projectSample projects tested:
ihp-gdsfactory--sample-projects/ihp--public--projectUsage:
Test plan
make test-gfp-projectsand verify all tests passSummary by Sourcery
New Features: