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

All-day breakfast at the dataprovider buffet #3401

Conversation

epdenouden
Copy link
Contributor

Thanks to @reinholdfuereder some remaining issues with the reordering of @dataprovider-fed tests have been resolved. As described in more detail #3396 and its predecessor ticket #3246, the main problem with execution reordering has been understanding the hierarchy of the run queue.

The fix

Lack of a consistent naming scheme for each individual combination of (test, data) reared its ugly head again. This has now been solved cleanly and I've updated all the tests to match the new scheme, so all previous functionality remains under guard. I have added a regression end-to-end test to keep an eye on the dataproviders.

Impact

  • failing dataprovider-fed tests are now sorted correctly within their TestSuite, in addition to the (already existing) functionality to sort failing dataprovider rows to the front of their parent test
  • previous execution order sorters are unaffected
  • the cache uses a different format to ID tests: NameSpace\TestClass::testMethod, NameSpace\TestClass::testMethod with data set $data or a PHPT filename
  • the TestSuiteSorter now uses normalized FQCN+dataname everywhere
  • the ResultCacheListener keeps the FQCN+dataname of tests but strips the dump of the data

Additional notes

I have opted to not add refinements to the TestResultCache at this time, to keep the changes atomic. If I learned anything from #3246 is that debugging the sorters is still very time consuming. Such optimizations add complexity and would only be noticeable by maintainers of large projects and even then it's more of a nice-to-have.

As always let me know if you have any remarks or wishes.

@codecov
Copy link

codecov bot commented Nov 13, 2018

Codecov Report

Merging #3401 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3401      +/-   ##
============================================
+ Coverage     83.05%   83.11%   +0.06%     
+ Complexity     3570     3567       -3     
============================================
  Files           143      143              
  Lines          9503     9502       -1     
============================================
+ Hits           7893     7898       +5     
+ Misses         1610     1604       -6
Impacted Files Coverage Δ Complexity Δ
src/Runner/ResultCacheExtension.php 100% <100%> (ø) 12 <0> (ø) ⬇️
src/Runner/TestSuiteSorter.php 99% <100%> (+0.97%) 45 <0> (-3) ⬇️
src/Util/TestDox/CliTestDoxPrinter.php 93.67% <0%> (+6.32%) 27% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdeb99a...c62d871. Read the comment docs.

@epdenouden epdenouden changed the title All day breakfast at the dataprovider buffet All-day breakfast at the dataprovider buffet Nov 13, 2018
@sebastianbergmann
Copy link
Owner

Merged into 7.4, thanks.

@epdenouden
Copy link
Contributor Author

@sebastianbergmann Thanks! This PR fixes #3246 and #3396, they can be closed, too

@epdenouden epdenouden deleted the issue-3396-all-day-breakfast branch December 5, 2018 19:11
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.

None yet

2 participants