-
Notifications
You must be signed in to change notification settings - Fork 887
Add relative path preference settings to PhpUnit #8478
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
Add relative path preference settings to PhpUnit #8478
Conversation
php/php.phpunit/src/org/netbeans/modules/php/phpunit/preferences/PhpUnitPreferences.java
Outdated
Show resolved
Hide resolved
...hp.phpunit/src/org/netbeans/modules/php/phpunit/preferences/PhpUnitPreferencesValidator.java
Outdated
Show resolved
Hide resolved
...hp.phpunit/src/org/netbeans/modules/php/phpunit/preferences/PhpUnitPreferencesValidator.java
Outdated
Show resolved
Hide resolved
@adamturcsan, sorry for the delay! Overall, the changes look OK to me; please, look into my comments. @junichi11, could you please verify it too? Thank you. |
I'm not feeling well, so please wait a bit, sorry... |
@junichi11 get well soon! 🤞 |
@tmysik Thank you, Tomas! |
2cfb49c
to
2525d5d
Compare
php/php.phpunit/src/org/netbeans/modules/php/phpunit/preferences/PhpUnitPreferences.java
Outdated
Show resolved
Hide resolved
php/php.phpunit/src/org/netbeans/modules/php/phpunit/commands/PhpUnit.java
Outdated
Show resolved
Hide resolved
php/php.phpunit/src/org/netbeans/modules/php/phpunit/preferences/PhpUnitPreferences.java
Outdated
Show resolved
Hide resolved
@adamturcsan, since @junichi11 is not feeling well, please, look into my recent comments (sorry, I totally missed that!) and once resolved, I will approve and we can merge it. If Junichi finds any problem later, we can easily create a ticket and fix it. Thank you. |
I'm sorry I'm late... @tmysik Thank you for the review! @adamturcsan Could you provide an example project as a zip file to check this change? |
@mbien Is this author name OK? |
i think so. The patch file is using mime encoding https://github.com/apache/netbeans/pull/8478.patch due to special characters which makes it look weird. But if you look at it using |
@mbien Thank you! |
I hope this project is okay like this. |
2525d5d
to
4a3400c
Compare
@adamturcsan Thanks. Could you write steps to run tests?
|
@junichi11 Sure. So my use case is driven by a dockerized setup, so the project I sent need docker.
You might want to run the tests without NB, then pls run I also updated the gaps around the new checkbox. |
@adamturcsan Thanks! I've verified that we can run tests after I add the user to the docker group.
Nice! Please squash commits as one commit and update the screenshot :) Your example project will be beneficial for anyone wanting to use this option. |
Nitpick: Maybe |
This checkbox gives boolean setting switching between two behaviour of passing test file paths to the PhpUnit command: FALSE = Use absolute paths [Current behaviour] TRUE = Use relative paths [Based on source root]
5554004
to
7e72192
Compare
@junichi11 Yes you're right, I forgot to mention the user. I've squashed the commits, updated the screenshot and removed the signed-off part. |
Just checked it once more and I don't see any problem now. @adamturcsan, great job, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thank you for your work :)
@tmysik Let's merge this if it's OK with you :) |
Yes, @adamturcsan, please merge this PR! 👍 |
@tmysik We have to merge this PR because @adamturcsan can't do it :) |
I see, sorry. I just wanted @adamturcsan to be more visible in the log history 🙂 |
@tmysik Need not apologize at all. I thought that you wanted to do that ;) Thanks! |
Thanks @junichi11 and @tmysik for the guidance and the review, this was my first PR here, so I can only hope I wasn't too much of a burden ❤️ 😅 |
@adamturcsan No problem :) Your PR was merged. Congrats🎉 |
@adamturcsan any PR is welcome, thank you for your work and time! |
This checkbox gives boolean setting switching between two behaviour of passing test file paths to the PhpUnit command:
FALSE/Unchecked = Use absolute paths [Current behaviour]
TRUE/Checked = Use relative paths
[Edit]
Motivation:
I develop PHP projects in a completely containerized setup. This setting would allow me to use all the functionality with a custom runner script, without dealing with path mapping, or any kind of more robust runner configuration.
^Add meaningful description above
Click to collapse/expand PR instructions
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log
) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)