Use XmlLint against tool configurations (PHPUnit, PHPCS, Psalm)
#31
Conversation
There was a problem hiding this comment.
My primary concern with this is that if these jobs fail... we've still queued jobs for the tools referenced, which means that they still run, but with known invalid schemas.
I think it might make more sense to inline the xmllint into the commands: xmllint ... && command .... This means that we fail early if the config is incorrect for the jobs running those tools.
|
Yeah I was looking into being able to set a job as dependent on another as I considered that a better approach. Do you have any ideas on that? Otherwise I will just implement the changes you requested. |
|
The problem with inlining the commands is what/how to do that with the PHPUnit matrix? That's why I considered a better solution to be allowing the jobs to be dependent on other jobs. |
|
@internalsystemerror I think it may make more sense to validate in this action; you can see how @boesing managed it for the |
On second thought, maybe not - many of these reference a schema file that's installed with the tooling, which happens via Composer. So, another possibility: do the linting as a precursor to running the associated command? |
|
I've fetched the latest changes since this was made, too. |
|
I have modified the commands to run |
|
Not 100% sure but if a project has both dist and non-dist, this would no create two jobs, right? Plenty of unrelated changes there as well. 😅 |
Correct, I believe it would.
Sorry, I have format on save. I've corrected these but left in the couple of syntax fixes it added. |
|
I wonder if it would be possible to avoid having two files created. Pseudo-Code: |
|
Nice idea, that would avoid 2 jobs being created if 2 files exist. Thinking about it though, if a second file is submitted (dist xml along with xml), either this is accidental, in which case having the same job twice would be an indication. Or, it's deliberate, in which case you may wish to run these separately? Honestly I don't see this latter one happening though. |
The only thing which came to my mind would be a Fun fact - we probably going to do exactly this for PHPUnit regarding the recent changes for deprecations to exceptions: laminas/laminas-continuous-integration-action#54 So in fact, we are not creating a new file from the |
There was a problem hiding this comment.
LGTM. Just a lil nitpick but from my perspective, this could work.
@weierophinney do you see anything else here? I think it would be fine if we spawn the Jobs and let them fail then since we need any feedback from the tooling.
Just not sure about the fact if we should provide the output from the linter to help devs fixing the problem
There was a problem hiding this comment.
Two categories of changes:
- Instead of
const filename = appendDistExtensionIfFilenameDoesntExist(...), name the constant something that is semantically appropriate: phpunitConfigFile, phpcsConfigFile, etc. - I'm thinking we shouldn't use the
--nooutflag. If we have--nooutand xmllint fails, it's not immediately obvious what happened. With output, the user will know what xmllint errored on.
This feature is based on the feedback from @Ocramius in laminas#76 and provides better namings to github UI. This will also prevent laminas#31 with its extended commands where we do check for linting before actually executing the command itself. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
|
The CI container now supports |
|
I've been looking at reimplementing this today, using the new Does anyone have any ideas on that? |
|
Wouldn't it be better to run an independent matrix job that only runs xmllint/yamllint for root level xml/yaml files? 🤔 A completely independent failure would be nicer than mixing it with test results, or worse with no tests run at all. |
Preventing tests from running with a known incorrect schema was requested in this comment #31 (review). Adding in these checks as just another linting check would certainly be the easiest solution to implement. |
I think the only way to do that is if we were to do this as one or more separate jobs in the workflow, marked to fail the workflow if it fails. Since we can extend workflows now, we could do this in the main workflow in the .github repo... |
|
I am not sure if its smart to have linting for configs seperately. I would expect all Jobs related to that Tool failing if the config is invalid rather than having another Job just to lint. So I would be +1 for adding these lint checks as a |
|
Works for me: no hard opinion, as long as linting manages to make it through, at least 👍 |
|
OK. Then since I have no preference either, I'll update this PR to simply add xmllint as a |
Signed-off-by: Gary Lockett <gary@creativecow.uk>
|
Forgive the incoming update spam. This took much longer than I want to admit, mainly because I can't get a reliable way to run the tests even on a clean branch. Pushing now to get test results via GHA, expect fixes (which is updating the expected |
Signed-off-by: Gary Lockett <gary@creativecow.uk>
|
Checks are green. Ready for review. @boesing tagging you specifically as you had some ideas already for this. Matrices that are generated for tools will include |
|
I hope I find some time this weekend to review this. And there is still that "if phpunit.xml exists, we should lint that file, else phpunit.xml.dist". But I would assume not to lint both at the same time for example. But I have no clue how to implement that properly, as the |
|
It's been waiting 15 months, another few weeks or even months wouldn't hurt it. The approach I took was to KISS. The tools are modified to reduce the Functionally it's the same as before, however the ability to add a tool in the future without any files to check has been removed. Then for each file to check that remains, if there is a linting command available it will add it as a Any more than that I found myself overengineering something that might not need to exist. For example, I'd also like to extend the before_script functionality to include explicit checks that have their own. |
| import {Action} from '../action'; | ||
| import {Output} from '../config/output'; | ||
| import {Logger} from '../logging'; | ||
|
|
||
| /* eslint-disable-next-line import/no-commonjs, @typescript-eslint/no-var-requires */ |
There was a problem hiding this comment.
I am pretty sure that I had to do this.
But since tests are passing, I think its okay now.
Maybe that was on my macos machine when I compiled that locally. Dunno...
Ocramius
changed the title
XmlLint against tool configurations (PHPUnit, PHPCS, Psalm)
Description
This PR adds the use of
xmllintif added via laminas/laminas-continuous-integration-action#37Note: I don't personally use psalm, and have not checked that the xmllint command works for this schema. However I obtained the schema from the
psalm.xmlofmezzio/mezzio, as I did with PHPUnit and PHPCS, both of which work correctly.