search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.64k stars 849 forks source link

Fix for PHPUnit 10 #5790 #5853

Closed junichi11 closed 1 year ago

junichi11 commented 1 year ago

nb-php-gh-5790-phpunit10-project-properties

junichi11 commented 1 year ago

The suite class file is checked whether it is a subclass of TestCase: https://github.com/sebastianbergmann/phpunit/blob/da707f3748773fbffd20289ae039e5e667d6b3a6/src/Runner/TestSuiteLoader.php#L42-L80

junichi11 commented 1 year ago

@tmysik What do you think? Any ideas?

tmysik commented 1 year ago

Overall looks good to me. Here are my notes:

Thanks!

junichi11 commented 1 year ago
  • cannot we use the PHPUnit 10 way also for older versions of PHPUnit? Maybe we could avoid the PHPUnit version selection at all?

Maybe, PHPUnit 9 works the same way (for a single file and directory). However, I'm not sure about the others...

  • cannot we detect what version of PHPUnit is used? I mean, some cheap way, like existence of some file inside PHPUnit directory (not so nice but could work)
  • cannot we improve error message so user knows what to do if the running of the tests does not work? Something like "Verify PHPUnit version in the Project Properties dialog"

I'll try thinking whether we can run unit test without selecting the version :) Thanks!

junichi11 commented 1 year ago

@tmysik We can detect the version via phpunit --version. What do you think?

I have no ideas about multiple files yet...

nb-php-fix-phpunit10

tmysik commented 1 year ago

@junichi11 The ideal solution, of course 👍

tmysik commented 1 year ago

@junichi11 Looks good to me. However, we still need to solve situation when several files should be tested (e.g. user selects more files, or re-run the failed tests).

junichi11 commented 1 year ago

@tmysik

However, we still need to solve situation when several files should be tested (e.g. user selects more files, or re-run the failed tests).

Yes. I'll submit that problem as a new issue after this is merged. (I have no ideas although I investigated a bit...) Thank you for your review!

tmysik commented 1 year ago

@junichi11 You are welcome!

junichi11 commented 1 year ago

I'll click the "Ready for review" button to merge this after CI passes. (This PR is merged by the release team.)

KacerCZ commented 1 year ago

I tried to find way how to pass multiple files to PHPUnit, but there is no easy way.

Possible solution could be inspired by https://github.com/infection/infection. It manipulates XML configuration for PHPUnit to suit its needs and creates temporary XML file. This file is then passed to PHPUnit with -c/--configuration parameter. See

Proposal

  1. NetBeans creates temporary XML file
    • if config file exists (specified in dialog configuration or default file name) then it copies its content
    • relative paths in new file needs to be updated for new location
    • if config file does not exist it creates new one
  2. create its own test suite with unique name (https://docs.phpunit.de/en/10.1/configuration.html#the-testsuite-element)
  3. add files/directories to the suite
  4. run PHPUnit with parameters --configuration and --testsuite (https://docs.phpunit.de/en/10.1/textui.html) to test selected files

What do you think?

tmysik commented 1 year ago

@KacerCZ

Thanks for looking into it! I was thinking about this solution also for the original solution (instead of NetBeansSuite.php), however, it cannot work in cases when the user already has any existing PHPUnit configuration. Or am I missing something?

cc @junichi11

tmysik commented 1 year ago

@KacerCZ

Let me improve my comment - I meant, can we always "extend" properly any existing configuration provided by the user? I am not so sure...

KacerCZ commented 1 year ago

Existing configuration file must be copied to temporary file and modified - add test suite and update relative paths.

Configuration file can be configured in NetBeans or PHPUnit is looking for phpunit.xml, phpunit.dist.xml or phpunit.xml.dist in working directory (root of project).

tmysik commented 1 year ago

Cannot the existing configuration contain some suite already? I mean, can we always update it? Sorry if it is clear and my questions are unnecessary 🙈

KacerCZ commented 1 year ago

@tmysik No need to apologize. Configuration file can contain one or more suites already defined. Idea is to add new test suite definition with and run this new suite.

Name of the suite could be something like "NetBeansTempSuite" with MD5 hash created from pathnames passed from Netbeans to prevent name collision with already existing suite.

tmysik commented 1 year ago

Ha, if we can add a suite, then it seems to me to be the best way to go. Thanks for clarification!

junichi11 commented 1 year ago

Thanks for investigating it. I'll submit that problem as a new issue later.

junichi11 commented 1 year ago

I noticed that only one file is run with also PHPUnit 9 when we select several files. (I assumed that we can do it, sorry.)

  1. Select two files (Tested files)
  2. Run > Test files

Am I wrong something?

Rerun failed: tests are run with --filter param

nb-php-phpunit-rerun-failed-result

nb-php-phpunit-rerun-failed-filter

tmysik commented 1 year ago

@junichi11

I can see 2 files were run, no?

junichi11 commented 1 year ago

@tmysik I should have written what I ran about that image, sorry. I selected the directory to show failed results for 2 files. I'm not sure how to run the selected 2 files actually. So, I've looked into it a bit.

https://github.com/apache/netbeans/blob/e65310a45ade2f08ce756536ce83bcc49237b101/php/php.project/src/org/netbeans/modules/php/project/ui/actions/support/CommandUtils.java#L299-L309

It seems that the first item is returned when we select several files.

https://github.com/apache/netbeans/blob/e65310a45ade2f08ce756536ce83bcc49237b101/php/php.project/src/org/netbeans/modules/php/project/ui/actions/support/ConfigActionTest.java#L172-L179

tmysik commented 1 year ago

Uff :) Sorry, it has been a long time... Cannot we call filesForContextOrSelectedNodes()? But maybe the current behavior is "good enough" and running tests in a folder is what users do? Anyway, thanks for looking into it.

junichi11 commented 1 year ago

Cannot we call filesForContextOrSelectedNodes()?

We should be able to do it. Then, we have to fix getTestRunInfoForFile() or add getTestRunInfoForFiles().

But maybe the current behavior is "good enough"

Yes, I also think so :)

and running tests in a folder is what users do?

I'm not sure... but maybe, usage is the following via CLI, so users didn't report anything about running several test files, I guess.

Usage:
  phpunit [options] UnitTest.php
  phpunit [options] <directory>

It doesn't seem a big problem for users (I think we need not hurry) because there is no report from users for a long time. At least, PHPUnit 10 should work fine with this PR if there are no wrong fixes in my changes :)

tmysik commented 1 year ago

I totally agree, not a big problem definitely.

BTW one more use-case is, if I remember it correctly 🙂, to rerun only failed tests. But that cannot work even now so all fine 👍 Let's create a ticket for it and merge this PR if there are no objections.

Thanks!

junichi11 commented 1 year ago

@tmysik I'll do that later :) This PR will be merged by the release team because I would like to include this in NB18. (We must not do it.) : https://lists.apache.org/thread/s3hnjtmr6t7dkyq1ofn4hdqmd0qh2d11 Thank you!

neilcsmith-net commented 1 year ago

This PR will be merged by the release team because I would like to include this in NB18.

Don't worry, I've been following the conversation! :smile:

I know it's been moved out of draft, but wasn't 100% sure from the ongoing conversation if it is ready to merge. Please confirm, and I'll merge by tomorrow in time for 18-rc2.

junichi11 commented 1 year ago

@neilcsmith-net Thank you!

junichi11 commented 1 year ago

Created the issue: https://github.com/apache/netbeans/issues/5880