-
Notifications
You must be signed in to change notification settings - Fork 1
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
#50 PHP-Scoper がテスト可能に #51
Changes from 12 commits
04ae20f
f6f4291
34f8e37
6b2758c
87e8427
3cd35dc
a6e9ee5
ac3cafa
c7357f1
cf05a2a
fa6c5ca
7f8c8c3
0df79ca
e9e8959
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
name: Setup PHP Scoper | ||
description: Setup PHP Scoper | ||
inputs: | ||
scoper: | ||
description: Whether to do PHP Scoper | ||
required: true | ||
Comment on lines
+1
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The required: true
default: 'no' And changing lines 12 and 21 to: if: ${{ inputs.scoper == 'yes' }} This ensures that the condition matches the input value and avoids any ambiguity. |
||
|
||
Comment on lines
+1
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The 3: inputs:
4: scoper:
5: description: Whether to enable PHP Scoper
6: required: false
7: default: false |
||
runs: | ||
using: "composite" | ||
Comment on lines
+8
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The
Comment on lines
+1
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The new hunk introduces a GitHub Actions workflow file for setting up PHP Scoper. It includes inputs for the |
||
steps: | ||
- name: Add Prefix | ||
if: 'yes' == inputs.scoper | ||
shell: bash | ||
run: | | ||
composer global require humbug/php-scoper | ||
~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor-prefixed --force | ||
composer dump-autoload --working-dir vendor-prefixed --classmap-authoritative | ||
composer scoper-patch | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The current implementation assumes that the PHP-Scoper tool is already installed globally. It would be better to include a step to install the tool if it's not already available. Additionally, the 11: - name: Install PHP Scoper
12: if: 'yes' == inputs.scoper
13: run: composer global require humbug/php-scoper
14:
15: - name: Add Prefix
16: if: 'yes' == inputs.scoper
17: shell: bash
18: run: |
19: ~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor/vendor-prefixed --force
20: composer dump-autoload --working-dir vendor/vendor-prefixed --classmap-authoritative
21: composer scoper-patch |
||
- name: Cleanup Vendor | ||
if: 'yes' == inputs.scoper | ||
run: rm -rf vendor | ||
shell: bash | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The cleanup step ( - name: Cleanup Vendor
if: ${{ inputs.scoper == 'yes' }}
run: rm -rf vendor
shell: bash This change ensures that the cleanup step is conditional and only executed when necessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The 20: - name: Cleanup Vendor
21: run: rm -rf vendor
22: shell: bash
23:
24: - name: Skip PHP Scoper |
||
- name: Skip PHP Scoper | ||
if: inputs.scoper != 'yes' | ||
run: echo "Skipping Scoper" | ||
shell: bash | ||
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The 25: - name: Skip PHP Scoper
26: run: echo "Skipping Scoper" Overall, here are the suggested changes: 1: name: Setup PHP Scoper
2: description: Setup PHP Scoper
3: inputs:
4: scoper:
5: description: Whether to enable PHP Scoper
6: required: false
7: default: false
8:
9: runs:
10: using: "composite"
11: steps:
12: - name: Install PHP Scoper
13: if: 'yes' == inputs.scoper
14: run: composer global require humbug/php-scoper
15:
16: - name: Add Prefix
17: if: 'yes' == inputs.scoper
18: shell: bash
19: run: |
20: ~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor/vendor-prefixed --force
21: composer dump-autoload --working-dir vendor/vendor-prefixed --classmap-authoritative
22: composer scoper-patch
23:
24: - name: Cleanup Vendor
25: run: rm -rf vendor
26:
27: - name: Skip PHP Scoper
28: run: echo "Skipping Scoper"
Comment on lines
+25
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The step named "Skip PHP Scoper" simply echoes "Skipping Scoper" if Overall, the changes look good. However, there are a few suggestions:
Here's an example of how the changes could be made:
Comment on lines
+25
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI Add a comment explaining why PHP Scoper is being skipped. - name: Skip PHP Scoper
if: inputs.scoper != 'yes'
run: echo "Skipping Scoper" # PHP Scoper is not enabled
shell: bash These suggestions will improve the clarity and robustness of the workflow. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,14 +12,13 @@ on: | |
|
||
jobs: | ||
test: | ||
runs-on: ${{ matrix.operating-system }} | ||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
operating-system: [ ubuntu-latest ] # OS. ubuntu-18.04 is also available. | ||
php: [ '7.2', '7.4', '8.0' ] # PHP versions to check. | ||
php: [ '7.4', '8.0' ] # PHP versions to check. | ||
wp: [ 'latest', '5.9' ] # WordPress version to check. | ||
tools: [ composer ] | ||
|
||
scoper: [ 'yes', 'no' ] | ||
services: | ||
mysql: | ||
image: mysql:8.0 | ||
|
@@ -29,9 +28,9 @@ jobs: | |
env: | ||
MYSQL_ROOT_PASSWORD: root | ||
MYSQL_AUTHENTICATION_PLUGIN: mysql_native_password | ||
name: UnitTest on WordPress ${{ matrix.wp }} in PHP ${{ matrix.php }} | ||
name: UnitTest WP ${{ matrix.wp }} in PHP ${{ matrix.php }} with PHP Scoper=${{ $matrix.scoper }} | ||
steps: | ||
- uses: actions/checkout@master | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Setup PHP with composer v2 | ||
uses: shivammathur/setup-php@v2 | ||
|
@@ -47,26 +46,59 @@ jobs: | |
- name: Install dependencies | ||
run: composer install --prefer-dist --no-progress --no-suggest | ||
Comment on lines
46
to
47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI Add the --no-dev flag to exclude development dependencies if they are not required for testing purposes. 50: 53: 57: |
||
|
||
Comment on lines
47
to
48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The step installs dependencies using Composer. No issues found. |
||
- name: Do PHP Scoper | ||
uses: ./.github/actions/php-scoper | ||
with: | ||
scoper: ${{ matrix.scoper }} | ||
|
||
- name: Make Binaries Executionable | ||
if: matrix.scoper == 'yes' | ||
run: chmod +x vendor-prefixed/vendor/bin/* | ||
|
||
- name: Start MySQL | ||
run: | | ||
sudo systemctl start mysql | ||
mysql -h 127.0.0.1 --port 3306 -u root --password=root -e "ALTER USER 'root'@'localhost' IDENTIFIED WITH mysql_native_password BY 'root';" | ||
Comment on lines
59
to
61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI A step is added to start MySQL and alter the root user's password. No issues found.
Comment on lines
60
to
61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI Ensure that the workflow environment has the necessary permissions to execute the sudo systemctl start mysql and mysql commands. 65: 115: 118: 120: 123:
|
||
|
||
|
||
- name: Install WordPress | ||
run: bash bin/install-wp-tests.sh wordpress root root 127.0.0.1:3306 ${{ matrix.wp }} | ||
Comment on lines
63
to
64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI A step is added to install WordPress for testing purposes. No issues found. |
||
|
||
Comment on lines
58
to
65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI Steps related to starting MySQL, installing WordPress, and running tests have been modified. There are no issues with these changes. |
||
- name: Run test suite | ||
- name: Run test suite with Vendor | ||
if: 'no' == matrix.scoper | ||
run: composer test | ||
|
||
- name: Run test suite with PHP Scoper | ||
if: 'yes' == matrix.scoper | ||
run: composer test:scoper | ||
|
||
lint: | ||
name: PHP Syntax check | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Setup PHP with composer v2 | ||
uses: shivammathur/setup-php@v2 | ||
with: | ||
php-version: 7.4 | ||
tools: composer | ||
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
||
- name: Validate composer.json and composer.lock | ||
run: composer validate | ||
|
||
- name: Install dependencies | ||
run: composer install --prefer-dist --no-progress --no-suggest | ||
|
||
- name: Check Lint | ||
run: composer lint | ||
|
||
assets: | ||
name: Check Assets | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@master | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Install Node | ||
uses: actions/setup-node@v3 | ||
Comment on lines
98
to
104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI A new job named "Check Assets" has been added to the workflow. It runs on
|
||
|
@@ -81,7 +113,7 @@ jobs: | |
|
||
status-check: | ||
name: Status Check | ||
needs: [ test, assets ] | ||
needs: [ test, lint, assets ] | ||
runs-on: ubuntu-latest | ||
steps: | ||
- name: Display Status | ||
|
@@ -93,7 +125,7 @@ jobs: | |
if: contains(github.ref, 'tags/') | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@master | ||
- uses: actions/checkout@v3 | ||
|
||
- name: Setup PHP with composer v2 | ||
uses: shivammathur/setup-php@v2 | ||
Comment on lines
125
to
131
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI A new job named "Build on Tags" has been added to the workflow. It runs only if the GitHub ref contains
|
||
|
@@ -103,15 +135,13 @@ jobs: | |
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
||
Comment on lines
135
to
137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI A new step is added to build the package. No issues found. |
||
- name: Build Package. | ||
- name: Build Package | ||
run: bash bin/build.sh | ||
|
||
- name: Do PHP scoper | ||
run: | | ||
composer global require humbug/php-scoper | ||
~/.composer/vendor/bin/php-scoper add-prefix --output-dir=vendor-prefixed --force | ||
composer dump-autoload --working-dir vendor-prefixed --classmap-authoritative | ||
composer help-scoper | ||
- uses: ./.github/actions/php-scoper | ||
name: Do PHP Scoper | ||
with: | ||
scoper: true | ||
|
||
- name: Clean Unwanted files | ||
run: bash bin/clean.sh | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI Several steps have been added to the "Build on Tags" job. Here are the key changes:
Overall, the changes in the workflow file seem fine. However, it's important to review the contents of the custom actions ( |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,14 +36,6 @@ | |
] ), | ||
], | ||
|
||
// List of excluded files, i.e. files for which the content will be left untouched. | ||
// Paths are relative to the configuration file unless if they are already absolute | ||
// | ||
// For more see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#patchers | ||
'exclude-files' => [ | ||
'src/a-whitelisted-file.php', | ||
], | ||
|
||
// When scoping PHP files, there will be scenarios where some of the code being scoped indirectly references the | ||
// original namespace. These will include, for example, strings or string manipulations. PHP-Scoper has limited | ||
// support for prefixing such strings. To circumvent that, you can define patchers to manipulate the file to your | ||
Comment on lines
36
to
41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The comment in the new hunk provides a good explanation of the purpose of the patchers. However, it seems that the old hunk is missing some lines of code that were removed. Without the complete context, it's difficult to determine if there are any issues with this change. |
||
|
@@ -60,14 +52,25 @@ static function ( string $filePath, string $prefix, string $contents ): string { | |
|
||
// For more information see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#excluded-symbols | ||
'exclude-namespaces' => [ | ||
'/^cli/', | ||
'/^Composer/', | ||
'/^DeepCopy/', | ||
'/^Doctrine/', | ||
'/^Kunoichi/', | ||
'/^PharIo/', | ||
'/^PHP_CodeSniffer/', | ||
'/^PHPCompatibility/', | ||
'/^PHPCSStandards/', | ||
'/^PHPUnit/', | ||
'/^Yoast/', | ||
'/^SebastianBergmann/', | ||
'/^TheSeer/', | ||
'/^WP_/', | ||
'/^Composer/', | ||
'/^cli/' | ||
], | ||
Comment on lines
52
to
69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The |
||
'exclude-classes' => [ | ||
'/^WP_/', | ||
'/^Composer/' | ||
'/^Composer/', | ||
'/^PHPUnit/', | ||
], | ||
Comment on lines
70
to
74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The |
||
'exclude-functions' => [ | ||
// 'mb_str_split', | ||
Comment on lines
52
to
76
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The new hunk updates the excluded namespaces, classes, and functions in the PHP-Scoper configuration file. It appears that the changes are intended to exclude additional namespaces, classes, and functions from being scoped. Without the complete context, it's difficult to determine if there are any issues with this change. |
||
|
@@ -80,7 +83,7 @@ static function ( string $filePath, string $prefix, string $contents ): string { | |
// | ||
// For more information see: https://github.com/humbug/php-scoper/blob/master/docs/configuration.md#exposed-symbols | ||
'expose-global-constants' => true, | ||
'expose-global-classes' => true, | ||
'expose-global-classes' => false, | ||
'expose-global-functions' => false, | ||
'expose-namespaces' => [ | ||
], | ||
Comment on lines
83
to
89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The new hunk modifies the values of 'expose-global-classes' and 'expose-global-functions' in the PHP-Scoper configuration file. The value of 'expose-global-classes' has been changed to false, and 'expose-global-functions' remains false. Without the complete context, it's difficult to determine if there are any issues with this change. |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,13 @@ | |
// Give access to tests_add_filter() function. | ||
require_once $_tests_dir . '/includes/functions.php'; | ||
|
||
Comment on lines
19
to
21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The comment on line 19 is unnecessary and can be removed. |
||
require_once dirname( __DIR__ ) . '/vendor/autoload.php'; | ||
/** | ||
* Manually load the plugin being tested. | ||
*/ | ||
function _manually_load_plugin() { | ||
require dirname( dirname( __FILE__ ) ) . '/ga-communicator.php'; | ||
} | ||
Comment on lines
+25
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤖 OpenAI The function /**
* Manually load the plugin being tested.
*/
function _manually_load_plugin() {
require dirname(dirname(__FILE__)) . '/ga-communicator.php';
} 28: // Load the plugin before running the tests.
tests_add_filter('muplugins_loaded', '_manually_load_plugin'); Please ensure that the code snippets provided above are placed in the correct location within the file. |
||
tests_add_filter( 'muplugins_loaded', '_manually_load_plugin' ); | ||
|
||
// Start up the WP testing environment. | ||
require $_tests_dir . '/includes/bootstrap.php'; |
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.
🤖 OpenAI
The
name
anddescription
fields in the GitHub Actions workflow file should be more descriptive. Consider updating them to provide clearer information about the purpose of the workflow.