Skip to content

Conversation

junseok44
Copy link
Contributor

@junseok44 junseok44 commented Sep 12, 2025

addresses #955 and is a follow-up to #959.

This PR improves error handling in the OpenProcessing API functions and adds tests, focusing on caching behavior, to prevent silent build issues.

Main changes

  • Added a validation filter in getStaticPaths() to exclude invalid sketch IDs.
  • Changed console.error to throw Error in getCurationSketches, getSketch, and getSketchSize to ensure failures are not ignored.

Previously, when OpenProcessing API calls failed, we would only log the errors and continue the build process with potentially broken data. This could lead to successful builds with broken sketch pages or cause confusing build failures much later in the pipeline. with the changes the build fails immediately and explicitly when an API call fails to make debugging much clearer and prevent broken pages from being deployed.

  • Added extensive tests for caching behavior and various error scenarios.

A test case for caching failures due to incorrect input types (e.g., passing a string to getSketch like #955) was intentionally omitted, as this type of error is better caught by static type checking.

Notes

With these changes, if an API call for an individual sketch fails, the entire build will now fail. We could adjust this in the future to skip only the problematic pages, but failing fast seems like a safer approach for now.

We should also consider making the CI build conditional on tests passing, especially since we are now testing caching behavior that is critical for build performance.

junseok44 and others added 4 commits September 12, 2025 14:58
- Add basic validation (visualID, type check) in sketch getStaticPaths
- Remove unnecessary isNaN check from SketchLayout
- Prevent invalid sketch pages from being generated during build
- Add comprehensive tests for getCurationSketches memoization
- Test getSketch cache usage and fallback to individual API calls
- Test getSketchSize API calls during build simulation
- Verify API call count regression to prevent performance issues
- Include test for individual sketch fetching when not in cache
- Replace console.error with throw Error in all OpenProcessing API functions
- Ensure build fails immediately when API calls fail instead of continuing with invalid data
- Add comprehensive error handling tests for all API functions
- Test various error scenarios: 500 Internal Server Error, 429 Rate Limit, 404 Not Found
- Improve build reliability by failing fast on API issues
- Export priorityIds for better test maintainability
@ksen0 ksen0 merged commit fe6cdb9 into processing:main Sep 17, 2025
4 checks passed
@ksen0
Copy link
Member

ksen0 commented Sep 17, 2025

Thank you @junseok44 for this excellent PR, the test suite is fantastic!
I'll merge this in but please feel free to respond below:

We should also consider making the CI build conditional on tests passing, especially since we are now testing caching behavior that is critical for build performance.

Unless I'm misunderstanding, if these tests fail, this will already stop the workflow and prevent deployment / will be clearly visible. Did you mean something additional as well? I agree this is critical functionality.

@junseok44
Copy link
Contributor Author

Thanks for reviewing! As you said, the deployment won't proceed if a test fails.
My concern was that the build job, running in parallel, could make excessive API calls (overfetching due to unexpected caching behavior) before the cache-test fails and stops the workflow. Although this is unlikely since the test usually runs faster than the build, it's a hassle to wait at least 10 minutes even if we get blocked by overfetching. so i thought making build job run after the test job would be better option in this case.

@ksen0
Copy link
Member

ksen0 commented Sep 18, 2025

Thanks for explaining @junseok44 , that seems totally reasonable, please feel free to open a PR adding the needs: test line to https://github.com/processing/p5.js-website/blob/main/.github/workflows/test.yml if you have time (referencing the original issue and this PR for context - since it's a followup)

@junseok44
Copy link
Contributor Author

sure, i've just added dependency on build in the PR above. please have a check!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants