-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[JsonPath] Use composer packages for JsonPath compliance test suite #62761
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.
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
base: 7.3
Are you sure you want to change the base?
[JsonPath] Use composer packages for JsonPath compliance test suite #62761
Conversation
4c3efd3 to
9fe24d8
Compare
src/Symfony/Component/JsonPath/Tests/JsonPathComplianceTestSuiteTest.php
Outdated
Show resolved
Hide resolved
9fe24d8 to
fae158c
Compare
fae158c to
cd375e3
Compare
|
instead of gitmodules, can't we use composer? I think we do so already for the intl component (or emoji? I don't remember) |
|
We could indeed use a |
|
I was thinking about the composer.json in this directory: |
f06914a to
49f764e
Compare
|
Thanks for the hints, I followed and implemented the Emoji component approach |
|
To me, the big difference between the way the Emoji component works and the way this PR works is that the Emoji composer-based task is about installing things needed for specific tasks updating committed data files that are part of the component. Running the testsuite does not require running those commands. In the case of the JsonPath testsuite (or the Yaml testsuite), the testsuite itself is a dev dependency. |
49f764e to
6f09fbf
Compare
|
Proposed a new approach in the last commit taking into account your comment @stof. Updating the test suite would need to edit two files, but I think it's worth it given how easy it makes it to install the suite. I'm not a big fan of the path guessing in the test case depending on if you use the monorepo or the standalone component, but I'm not sure there's a better solution here? |
c7fc77c to
b96ee82
Compare
src/Symfony/Component/JsonPath/Tests/JsonPathComplianceTestSuiteTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/JsonPath/Tests/JsonPathComplianceTestSuiteTest.php
Outdated
Show resolved
Hide resolved
| "source": { | ||
| "type": "git", | ||
| "url": "https://github.com/jsonpath-standard/jsonpath-compliance-test-suite.git", | ||
| "reference": "b9d7153e58711ad38bb8e35ece69c13f4b2f7d63" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about referencing a branch? is that possible? so that we won't have to change this file to get updates?
same below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible but explicitly discouraged by Composer (first note block of https://getcomposer.org/doc/05-repositories.md#package-2). Also, the version field is mandatory so this would require to put some placeholder (e.g. 1.0.0) that would be completely independent from the distant repository (which don't provide tags). These are quite a few reasons to stick with the current approach even if it won't update automatically, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas this won't work fine:
- if you already have it locally, it will never be updated unless you delete your vendor folder
- our testsuite won't be reproducible anymore (while we have a setup to skip some tests that are expected to not be passing yet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we need some documentation about the process.
b96ee82 to
6a4513c
Compare
I'd suggest considering this one as a bug fix so all supported branches benefit from this.