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

Refactor some mapping tests for improved coverage and forward compat #2732

Merged

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Dec 4, 2023

Ref: #2708

Right now there are a lot of tests that rely on the deprecated annotation and YAML drivers from the ORM to actually test anything. This means in a lot of cases the XML mapping isn't explicitly tested in a mapping case (it is incidentally covered elsewhere), nor can the tests run without the deprecated Annotations package. This is a first step at improving some of the test cases to be able to cover XML and explicitly test attribute mapping (and while all the extension-level attribute drivers are just an extension of their respective annotation drivers, there should be coverage when working purely in the context of attributes and not using the annotations package at all).

This PR refactors the loggable, sluggable, and soft-deleteable mapping tests to conditionally test mapping drivers based on whether the underlying dependencies are available so that the tests automatically skip themselves when needed while also supporting covering all mapping options. I'm mostly building on and re-using existing test fixtures where they exist, and adding attributes where they don't exist on annotated classes, so some stuff might not exist at all right now (i.e. there isn't an XML mapping or fixture class for the Category models tested with other drivers). Following this pattern for at least the mapping tests, this will help ensure explicit coverage on all drivers while only testing drivers that the environment supports.

@mbabker mbabker force-pushed the mapping-test-refactoring branch 2 times, most recently from b3b1f57 to 4ec7bde Compare December 4, 2023 23:15
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (3b5b5cb) 79.27% compared to head (f5224ba) 79.40%.
Report is 1 commits behind head on main.

Files Patch % Lines
src/Mapping/Event/Adapter/ORM.php 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2732      +/-   ##
==========================================
+ Coverage   79.27%   79.40%   +0.13%     
==========================================
  Files         162      162              
  Lines        8467     8494      +27     
==========================================
+ Hits         6712     6745      +33     
+ Misses       1755     1749       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -570,6 +570,11 @@ parameters:
count: 3
path: src/Uploadable/UploadableListener.php

-
message: "#^Class Gedmo\\\\Tests\\\\Translatable\\\\Fixture\\\\CategoryTranslation not found\\.$#"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Baselining this because the YAML mapping that the annotation/attribute mappings were generated from references this non-existing class, and PHPStan now (rightfully) groans about it. The test fixtures should probably use an existing class at some point in the future, but for now, its absence doesn't break anything.

@franmomu franmomu merged commit 0f00ca2 into doctrine-extensions:main Dec 5, 2023
18 checks passed
@franmomu
Copy link
Collaborator

franmomu commented Dec 5, 2023

thanks @mbabker!

@mbabker mbabker deleted the mapping-test-refactoring branch December 5, 2023 14:30
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.

2 participants