Add jest and a simple test example #136
Conversation
Signed-off-by: Gary Lockett <gary@creativecow.uk>
|
@Ocramius I think we have possibly found the cause of the issue on @boesing I know you've done a lot of work on this, so I wanted to set up unit testing and ping you so you're kept in the loop. |
Signed-off-by: Gary Lockett <gary@creativecow.uk>
| const phpIniFromConfigurationPath: PathLike = 'tests/php-ini-from-configuration'; | ||
|
|
||
| it('should return valid config', () => { | ||
| expect(createConfig( |
There was a problem hiding this comment.
Overall okay with this testing approach, but note that we could've caught this with the previous testing approach too 👍
There was a problem hiding this comment.
I think of the current approach as higher up the pyramid. Unit testing will give us quicker clarity as to what the issue is, so the more the better I say.
| codeChecks : true, | ||
| docLinting : true, | ||
| }, | ||
| `${phpIniFromConfigurationPath}/composer.json`, |
There was a problem hiding this comment.
What are we testing here? 🤔
Would probably make sense to have these files readable inside the test?
There was a problem hiding this comment.
Yeah, this was more just a quick example, but it makes sense to have distinct tests. Ideally I'd have passed in the configurations themselves, but these needed filenames so I used existing ones.
| )).toEqual({ | ||
| codeChecks : true, | ||
| docLinting : true, | ||
| versions : [ '8.1' ], |
There was a problem hiding this comment.
One thing I realized: ~8.1.2 will, for example, never match 8.1 🤔
There was a problem hiding this comment.
It's normalizing all the versions to these minor version numbers beforehand, so it should in theory:
laminas-ci-matrix-action/src/config/app.ts
Lines 30 to 31 in d9110cb
There was a problem hiding this comment.
Do we have an existing test for that scenario?
There was a problem hiding this comment.
Good question, I'm not even sure what's supposed to be returned here, I'm taking the code at face value. Could do with waiting for @boesing (time permitting obviously) to have a look over this, and I can add more tests after that.
There was a problem hiding this comment.
I associated many PRs that are affected by this specific problem: they can be seen on the PR overview.
The problem is that a dependency range like "php": "~8.1.12" will produce a matrix containing PHP 7.4, 8.0, 8.1, for some reason.
There was a problem hiding this comment.
It was the line mentioned above after all. composerPhpVersion is the constraint pulled straight from require.php in composer.json.
Signed-off-by: Gary Lockett <gary@creativecow.uk>
Signed-off-by: Gary Lockett <gary@creativecow.uk>
internalsystemerror
force-pushed
the
add-unit-testing
branch
from
f2d4839 to
5d30f24
Compare
Signed-off-by: Gary Lockett <gary@creativecow.uk>
|
Closing in favour of #139 |
Description
In order to investigate laminas/automatic-releases#212, and as previously requested by @boesing we need to add unit tests.
In adding a simple example for
config/app.ts#createConfig()it appears that thestableVersionfor8.1is actually7.4which may be the cause of the issue above.