Skip to content
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

API Selectively register test folder in manifest #10150

Merged

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Nov 15, 2021

Copy link
Contributor Author

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a work in progress, but most of the needed logic as been implemented ... it's just a question of updating Kernel and the Manifests to use it.

A question worth pondering is what we do with PHPUnit version 4, 6, 7, 8 or 10. Right now, I'm treating those as "unknown" ... this means those test will be included when running PHPUnit 5 and excluded when running PHPUnit 9. If we add support for PHPUnit 10 in a later release, we'll have to tweak this logic again.

A thing to note is that the new logic will only apply to tests folders that are at least 3 levels down (aka inside the vendor folder) ... if you write your tests using the PHPUnit 5 API and decide to install PHPUnnit 9, I'm not sure what we can do for you.

composer.json Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
src/Core/Manifest/ManifestFileFinder.php Outdated Show resolved Hide resolved
src/Core/Manifest/ManifestFileFinder.php Outdated Show resolved Hide resolved
src/Core/Manifest/ManifestFileFinder.php Outdated Show resolved Hide resolved
src/Core/Manifest/Module.php Outdated Show resolved Hide resolved
src/Core/Manifest/Module.php Show resolved Hide resolved
src/Core/Manifest/Module.php Outdated Show resolved Hide resolved
@maxime-rainville maxime-rainville marked this pull request as ready for review November 16, 2021 23:42
src/Core/Manifest/ClassLoader.php Outdated Show resolved Hide resolved
src/Dev/TestKernel.php Outdated Show resolved Hide resolved
src/Core/Manifest/ClassManifest.php Show resolved Hide resolved
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks great

I think the term 'CI Library' sounds, weird/wrong? Something along the lines of "Module test folder' is more correct?

src/Core/Manifest/Module.php Outdated Show resolved Hide resolved
src/Core/Manifest/Module.php Outdated Show resolved Hide resolved
src/Core/Manifest/Module.php Outdated Show resolved Hide resolved
@maxime-rainville
Copy link
Contributor Author

Ready for (hopefully) final review.

  • I've rebased back on the 4 branch
  • I've renamed everything from CiLibrary to CiConfig
  • Module::getCIConfig() now returns an arry.
  • I've add regex after the constraint check to cover things like 1.2.3, ~1.2.3, 1.2.*. Added more unit tests to cover those extra edge cases.

@maxime-rainville
Copy link
Contributor Author

I've implemented @michalkleiner suggestions.

@emteknetnz
Copy link
Member

Merge on green

@maxime-rainville maxime-rainville merged commit ae5aa4b into silverstripe:4 Nov 21, 2021
@maxime-rainville maxime-rainville deleted the pulls/4/manifest-destiny branch November 21, 2021 22:27
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.

3 participants