Support running against Phel#873
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Phel (PHP) as a supported dialect for running the Clojure core test suite, including CI coverage and documentation to set up the Phel environment locally.
Changes:
- Add Phel-specific reader-conditional branches across many core tests to account for missing/variant features (vars, ratios, MapEntry representation, numeric predicates, etc.).
- Introduce Composer + Phel configuration/docs for running the suite under PHP.
- Extend GitHub Actions CI to run the suite via
phel teston a PHP version matrix.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/clojure/core_test/vec.cljc | Skip/adjust array aliasing expectations for Phel. |
| test/clojure/core_test/var_qmark.cljc | Skip var? tests on Phel (no vars yet). |
| test/clojure/core_test/val.cljc | Adjust MapEntry representation for Phel. |
| test/clojure/core_test/uuid_qmark.cljc | Adjust non-UUID cases for Phel UUID string behavior. |
| test/clojure/core_test/taps.cljc | Treat Phel awaitables as pending for tap delivery behavior. |
| test/clojure/core_test/symbol.cljc | Skip var-based symbol tests for Phel. |
| test/clojure/core_test/string_qmark.cljc | Add Phel-specific cases for class/string and char behavior. |
| test/clojure/core_test/str.cljc | Adjust decimal string formatting expectations for Phel. |
| test/clojure/core_test/star.cljc | Skip rational tests for Phel. |
| test/clojure/core_test/special_symbol_qmark.cljc | Adjust special symbol set to match Phel capabilities. |
| test/clojure/core_test/slash.cljc | Adjust multi-arg division and skip rational tests for Phel. |
| test/clojure/core_test/short.cljc | Skip short-specific instance checks for Phel. |
| test/clojure/core_test/seqable_qmark.cljc | Mark chars seqable? for Phel. |
| test/clojure/core_test/repeat.cljc | Adjust fractional repeat behavior for Phel. |
| test/clojure/core_test/remove_watch.cljc | Skip var/ref/agent watch tests for Phel. |
| test/clojure/core_test/reduce.cljc | Add Phel numeric interop mappings and expected results. |
| test/clojure/core_test/ratio_qmark.cljc | Skip ratio tests for Phel. |
| test/clojure/core_test/print_str.cljc | Align print-str output expectations for Phel. |
| test/clojure/core_test/pos_int_qmark.cljc | Adjust bigint classification for Phel. |
| test/clojure/core_test/portability.cljc | Add Phel branches for big-int? logic, sleep, and thrown? catching. |
| test/clojure/core_test/plus.cljc | Skip rational tests for Phel. |
| test/clojure/core_test/parents.cljc | Add Phel class/type inheritance parent expectations. |
| test/clojure/core_test/neg_int_qmark.cljc | Adjust bigint classification for Phel. |
| test/clojure/core_test/minus.cljc | Skip rational tests for Phel. |
| test/clojure/core_test/key.cljc | Adjust MapEntry representation for Phel. |
| test/clojure/core_test/integer_qmark.cljc | Adjust integer? expectations around ratios for Phel. |
| test/clojure/core_test/int_qmark.cljc | Adjust int? expectations for bigints/ratios in Phel. |
| test/clojure/core_test/inc.cljc | Add Phel overflow behavior expectation. |
| test/clojure/core_test/float_qmark.cljc | Adjust float? expectations for Phel decimals/ratios. |
| test/clojure/core_test/float.cljc | Adjust float behavior expectations for Phel. |
| test/clojure/core_test/eq.cljc | Treat regex eq semantics as true for Phel. |
| test/clojure/core_test/double_qmark.cljc | Adjust double? expectations for Phel. |
| test/clojure/core_test/derive.cljc | Use Phel class tags for derive/isa? tests. |
| test/clojure/core_test/dec.cljc | Add Phel underflow expectation (currently flawed per review). |
| test/clojure/core_test/char_qmark.cljc | Add Phel-specific char? expectations. |
| test/clojure/core_test/case.cljc | Add Phel-specific case dispatch expectations for numeric types. |
| test/clojure/core_test/associative_qmark.cljc | Adjust associative? expectations for Phel seq/arrays. |
| test/clojure/core_test/ancestors.cljc | Add Phel class/type inheritance expectations (contains a malformed conditional per review). |
| test/clojure/core_test/add_watch.cljc | Adjust exception class and skip unsupported watch cases for Phel. |
| test/clojure/core_test/aclone.cljc | Adjust aclone identity expectations for Phel array semantics. |
| phel-config.php | Configure Phel test directory to use repo’s test/. |
| doc/phel.md | Document how to install/run tests and update Phel via Composer. |
| composer.json | Add Composer dependency on phel-lang/phel-lang. |
| README.md | Link to Phel setup documentation. |
| .gitignore | Ignore Composer vendor/ directory. |
| .github/workflows/ci.yml | Add a test-phel CI job running Phel tests on a PHP matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jasalt
force-pushed
the
main
branch
2 times, most recently
from
baeedd8 to
a148a09
Compare
|
Fixed the typos and did couple suggested changes. Thank's. CI runs ok https://github.com/phel-lang/clojure-test-suite/actions/runs/25230564223 |
| #?(:clj (is (p/thrown? (dec Long/MIN_VALUE))) | ||
| :cljr (is (p/thrown? (dec Int64/MinValue))) | ||
| :cljs (is (= (dec js/Number.MIN_SAFE_INTEGER) (- js/Number.MIN_SAFE_INTEGER 2))) | ||
| :phel (is (= (dec php/PHP_INT_MIN) (dec php/PHP_INT_MIN))) |
There was a problem hiding this comment.
@jeaye not for this PR but I think we should move minimum/maximum values into the portability namespace
There was a problem hiding this comment.
Wait we already do this: https://github.com/jank-lang/clojure-test-suite/blob/main/test/clojure/core_test/number_range.cljc
I'll make an issue and self assign, should be an easy cleanup @jasalt don't worry about it for this PR
There was a problem hiding this comment.
Yea, it's easy to miss this number_range.cljc namespace. Could be nice to have both it and the portability.cljc (and possible other utility namespaces) separated from tests in e.g. the parent or sibling dir.
There was a problem hiding this comment.
yeah I've thought about this, @jeaye what do you think?
There was a problem hiding this comment.
I agree that it'd be cleaner to separate them from the test files. I also think that we should end up combingin number_range and portability into just portability.
| :cljs js/Boolean | ||
| :lpy python/bool)}) | ||
| :lpy python/bool | ||
| :phel php/boolval)}) |
There was a problem hiding this comment.
@jeaye also think these are ripe for portability cleanup potentially but not sure how you feel (again not blocking this PR by any means)
There was a problem hiding this comment.
Yep, agreed. If we can remove this sort of stuff from test files so that they can just focus on the tests, we win.
E-A-Griffin
left a comment
E-A-Griffin
left a comment
There was a problem hiding this comment.
Mostly looks great! Some feedback here, some of this you could push back on also curious on @jeaye 's take on this, but nothing too serious here
Clojure-like Lisp compiling to PHP For non-squashed commit history see archived branch at: https://github.com/phel-lang/clojure-test-suite/tree/phel-integration Co-authored-by: Chemaclass <chemaclass@outlook.es> Co-authored-by: Jesus Valera Reales <github@jesusvalera.dev>
|
Thank's, I resolved these mostly as suggested and did more noticeable changes in tests regarding rationals in Phel does support rational literals calculating them as float, so instead of skipping the tests altogether like CLJS, I adapted the It's bit verbose being mostly copy/paste, but I wanted to avoid tinkering with the Other opinions? Passing CI run https://github.com/phel-lang/clojure-test-suite/actions/runs/25366677066 |
|
Vars were just added to Phel. I’ll mark this draft until I revisit a couple tests today relying on them. |
jasalt
force-pushed
the
main
branch
3 times, most recently
from
de61aef to
b2af8ec
Compare
|
Alright, a couple more Clojure features were addded to Phel and now tests here have been updated accordingly with some earlier Phel specific overrides having been discarded. Test suite is giving green again with 4665 tests ran (CI run). |
E-A-Griffin
left a comment
E-A-Griffin
left a comment
There was a problem hiding this comment.
Looks good to me! Obviously get feedback from the other reviewers first
There was a problem hiding this comment.
I will create tomorrow a new phel release with all these stable improvements! This PR brought so many ideas into phel! UPDATE: https://github.com/phel-lang/phel-lang/releases/tag/v0.36.0
|
Nice work! Are you folks all set to merge this? |
dgr
left a comment
dgr
left a comment
There was a problem hiding this comment.
Looks good to me. Merge it.
|
Can't now. There are conflicts! I don't get pinged when people give a 👍 on a comment, so I didn't know anyone responded to my question. We need to resolve the conflicts and then we're good to go. |
|
I am on it |
|
|
- drop nonexistent src dir - align formatDirs with test/ - enable deprecation warnings - add composer test script
|
Ok, I am done with the updates for now. Conflicts fixed and docs updated. Looking forward to see this merged 🥳 |
- Add `bb test-phel` task; include in `test-all` and dialect picker. - Cache Composer downloads and `.phel/cache` in the test-phel CI job. - CI now runs `composer test` so XDEBUG_MODE=off lives in one place.
|
Nice work, guys! |
|
Phel Team, in the future, I'd like to ask you to please submit separate PRs for individual units of work. This felt like a bit like a whole wad of stuff that was constantly changing as you guys were adding features to Phel. Further, things like the bb task slipped in at the last minute, etc. It would be nice to have a PR that is relatively contained and stays somewhat static during the PR review. Sure, change things in response to review feedback or bugs you find. But if you have units of work, feel free to submit multiple PRs. That would feel a lot less chaotic to me as a reviewer. Thanks. |
|
Absolutely, @dgr . That's way easier and more practical. Sorry for the extra complexity added for the review and thanks again for the check and merge :) |
Includes changes from https://github.com/phel-lang/clojure-test-suite/tree/phel-integration as a single squashed commit.
Open question: include both 8.4 and 8.5 in test matrix? Background in Slack https://clojurians.slack.com/archives/C0A2CQQQVPB/p1777553801232369?thread_ts=1775891783.135539&cid=C0A2CQQQVPB