Opened 2 years ago
Last modified 8 months ago
#56615 reopened defect (bug)
Running `build` scripts on Windows machines produces different results
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 6.1 |
Component: | Build/Test Tools | Keywords: | needs-testing needs-patch |
Focuses: | Cc: |
Description
When running npm run build:dev
on a Windows machine, the hashes calculated by Webpack are different and not as expected.
This seems to be related to an encoding issue with how the remove-accents
dependency is read and compiled.
Slack discussion thread: https://wordpress.slack.com/archives/C02RQBWTW/p1663687669117549?thread_ts=1663687642.461539&cid=C02RQBWTW
Change History (55)
This ticket was mentioned in Slack in #core by desrosj. View the logs.
2 years ago
This ticket was mentioned in PR #3309 on WordPress/wordpress-develop by gziolo.
2 years ago
#5
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/56615
2 years ago
#6
It's based on the finding from @jsnajdr that stable hashes are only guaranteed for the production version of the build.
I did some more debugging today and it seems that not even production build reliably guarantees stable order of modules and stable hashes. When the chunk modules are written to a file, they are ordered by module.identifier()
, which is not the thing that you defined in optimization.moduleIds
, e.g., named
or deterministic
, but the module's request
field, which is the full absolute path to the module, like /Users/jsnajdr/project/src/index.js
. See this webpack method that returns this.request
and doesn't really look at anything else:
The real reason why production builds are OK is that production uses module concatenation. The offending date-fns
modules are concatenated into another module and are no longer written separately. Then the unstable sorting doesn't happen.
I'll be investigating further in cooperation with @ockham
2 years ago
#7
I'll be investigating further in cooperation with @ockham
Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.
In the meantime, I applied all the necessary changes to make CI happy. I think it's a good improvement on its own as it allows us to keep only the file with combined assets for production to ensure that they never differ between platforms.
2 years ago
#8
I'll be investigating further in cooperation with @ockham
Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.
In the meantime, I applied all the necessary changes to make CI happy. I think it's a good improvement on its own as it allows us to keep only the file with combined assets for production to ensure that they never differ between platforms.
#10
@
2 years ago
- Keywords needs-testing added
It would be great to test the changes proposed with a fresh repository on Windows and Unix to ensure that unit tests pass.
2 years ago
#11
I'll be investigating further in cooperation with @ockham
Thank you so much for sharing your findings. That would be great to confirm it won't cause similar issues in the future.
Update: @jsnajdr prepared a minimal test case that I ran on Windows. He'll use the results to file a bug report against Webpack.
2 years ago
#12
Webpack bug reported here: https://github.com/webpack/webpack/issues/16289
2 years ago
#14
Committed with https://core.trac.wordpress.org/changeset/54289.
#15
follow-up:
↓ 16
@
2 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
Eh... I'm not sure what you all were trying to do here, but commit [54289] breaks three out of the four previously supported test workflows.
I won't bore you with the full error log, but the short version comes down to this:
Warning: include(path/to/WP/src/wp-includes/assets/script-loader-react-refresh-entry.php): Failed to open stream: No such file or directory in path/to/WP/src/wp-includes/script-loader.php on line 241 Warning: include(): Failed opening 'path/to/WP/src/wp-includes/assets/script-loader-react-refresh-entry.php' for inclusion (include_path='.;C:\php\pear') in path/to/WP/src/wp-includes/script-loader.php on line 241 Warning: include(path/to/WP/src/wp-includes/assets/script-loader-packages.php): Failed to open stream: No such file or directory in path/to/WP/src/wp-includes/script-loader.php on line 276 Warning: include(): Failed opening 'path/to/WP/src/wp-includes/assets/script-loader-packages.php' for inclusion (include_path='.;C:\php\pear') in path/to/WP/src/wp-includes/script-loader.php on line 27 6 Warning: foreach() argument must be of type array|object, bool given in path/to/WP/src/wp-includes/script-loader.php on line 278
This also impacts integration tests for plugins and themes, so as far as I'm concerned, this commit needs to be reverted until a more stable solution has been created.
#16
in reply to:
↑ 15
@
2 years ago
Thank you for sharing all the details, @jrf. It means that those workflows for unit tests match the following conditions:
SCRIPT_DEBUG
is defined and set totrue
- same as when running units onwordpress-develop
CIWP_RUN_CORE_TESTS
is not defined – that would be the difference
We can bring back (remove from git and svn ignore list) all the generated files by npm run build:dev
that are missing for the use cases you mentioned.
The alternative would be identifying a constant or a condition that is common for all test workflows, so we don't use dev files there that don't have any impact on unit tests.
#17
@
2 years ago
@gziolo I can also imagine that this could be solvable by having a check in the test bootstrap.php
file for these files and creating a dummy file for each of them if they don't exist ?
More than anything, it concerns me that this change was made without taking integration tests from plugins/themes into account.
#18
@
2 years ago
Thanks for already reporting the consequences of this change, @jrf. I can confirm that this now raises PHP warnings in my plugin's unit test setup with those include()
warnings.
This ticket was mentioned in Slack in #core-committers by gziolo. View the logs.
2 years ago
This ticket was mentioned in PR #3340 on WordPress/wordpress-develop by gziolo.
2 years ago
#20
Trac ticket: https://core.trac.wordpress.org/ticket/56615
#21
follow-up:
↓ 22
@
2 years ago
I have a patch https://github.com/WordPress/wordpress-develop/pull/3340 ready after discussing different options with @hellofromTonya.
#22
in reply to:
↑ 21
@
2 years ago
Replying to gziolo:
I have a patch https://github.com/WordPress/wordpress-develop/pull/3340 ready after discussing different options with @hellofromTonya.
Thanks for working on this @gziolo ! I've left some questions in the PR and tested the patch and can confirm it would fix the issue.
hellofromtonya commented on PR #3340:
2 years ago
#23
Self-assigning to apply requested changes, test, and prep commit.
hellofromtonya commented on PR #3340:
2 years ago
#24
Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of trunk
):
1) Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency Failed asserting that two strings are identical. --- Expected +++ Actual @@ @@ console.log("before"); </script> <script type='text/javascript' src='http://example.com' id='test-example-js'></script> -<script type='text/javascript' src='/wp-includes/js/dist/i18n.min.js' id='wp-i18n-js'></script> +<script type='text/javascript' src='/wp-includes/js/dist/i18n.js' id='wp-i18n-js'></script> <script type='text/javascript' id='wp-i18n-js-after'> wp.i18n.setLocaleData( { 'text direction\u0004ltr': [ 'ltr' ] } ); </script> -<script type='text/javascript' src='/wp-includes/js/dist/a11y.min.js' id='wp-a11y-js'></script> +<script type='text/javascript' src='/wp-includes/js/dist/a11y.js' id='wp-a11y-js'></script> <script type='text/javascript' src='http://example2.com' id='test-example2-js'></script> <script type='text/javascript' id='test-example2-js-after'> console.log("after"); </script> ' /var/www/tests/phpunit/includes/abstract-testcase.php:915 /var/www/tests/phpunit/tests/dependencies/scripts.php:761 phpvfscomposer:///var/www/vendor/phpunit/phpunit/phpunit:97
2 years ago
#25
Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of trunk):
I think it might happen when you run npm run build:dev
locally which effectively creates the asset file used for dev purposes. In that case, it serves JS files in dev mode with unit tests ... Maybe we should keep the check for core unit tests, too?
2 years ago
#26
Maybe we should keep the check for core unit tests, too?
That's almost certainly the wrong solution direction as anything that depends on checking WP_RUN_CORE_TESTS
to _not_ cause failures, _will_ lead to failures in plugin/theme integration test suites.
2 years ago
#27
Hmm, this PR is failing on my machine when running locally (before and after the force push - rebase on top of
trunk
):
1) Tests_Dependencies_Scripts::test_wp_add_inline_script_before_after_concat_with_core_dependency Failed asserting that two strings are identical.Investigating.
I just tried to reproduce this on my machine and the tests seemed to run fine..., but I hadn't run npm
in a long time, so I ran the npm install
and npm run build:dev
commands, then after downgrading node and npm and finally getting the bloody commands to work, I got the same result.
Checked against `trunk and the tests pass, so it is something to do with this PR.
Env which I used to run the tests:
- Windows 10
- PHP 8.2
- PHPUnit 9.5.25 PHAR
All the more interesting that the tests seems to pass in CI, which is yet another reason we need to add a wider variety of test envs in CI and not rely on Docker.
Looking at the test failure, I think this needs to be fixed in the tests by setting better expectations. @hellofromtonya Have you got a fix lined up already or shall I push one ?
2 years ago
#28
As far as I'm concerned this is the patch needed:
{{{diff
--- a/tests/phpunit/tests/dependencies/scripts.php
+++ b/tests/phpunit/tests/dependencies/scripts.php
@@ -720,17 +720,18 @@ JS;
$wp_scripts->base_url = ;
$wp_scripts->do_concat = true;
+ $suffix = wp_scripts_get_suffix();
$ver = get_bloginfo( 'version' );
$suffix = wp_scripts_get_suffix();
$expected = "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&load%5Bchunk_0%5D=jquery-core,jquery-migrate,regenerator-runtime,wp-polyfill,wp-dom-ready,wp-hooks&ver={$ver}'></script>\n";
$expected .= "<script type='text/javascript' id='test-example-js-before'>\nconsole.log(\"before\");\n</script>\n";
$expected .= "<script type='text/javascript' src='http://example.com' id='test-example-js'></script>\n";
- $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/i18n.min.js' id='wp-i18n-js'></script>\n";
+ $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/i18n{$suffix}.js' id='wp-i18n-js'></script>\n";
$expected .= "<script type='text/javascript' id='wp-i18n-js-after'>\n";
$expected .= "wp.i18n.setLocaleData( { 'text direction\u0004ltr': [ 'ltr' ] } );\n";
$expected .= "</script>\n";
- $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/a11y.min.js' id='wp-a11y-js'></script>\n";
+ $expected .= "<script type='text/javascript' src='/wp-includes/js/dist/a11y{$suffix}.js' id='wp-a11y-js'></script>\n";
$expected .= "<script type='text/javascript' src='http://example2.com' id='test-example2-js'></script>\n";
$expected .= "<script type='text/javascript' id='test-example2-js-after'>\nconsole.log(\"after\");\n</script>\n";
}}}
hellofromtonya commented on PR #3340:
2 years ago
#29
I agree with the change @jrfnl recommended. However, that change does not resolve the issue for me when running tests locally. Test still fails.
hellofromtonya commented on PR #3340:
2 years ago
#30
Checked against `trunk and the tests pass, so it is something to do with this PR.
Ran tests locally against current trunk
- all passed ✅
Ran tests locally against forked trunk
- all passed ✅
The issue is being introduced in this PR.
hellofromtonya commented on PR #3340:
2 years ago
#31
Good > CI PHPUnit tests are now failing after https://github.com/WordPress/wordpress-develop/pull/3340/commits/ed226e8063f6ac566b669d0969e9d44d1d8bf5de
2 years ago
#32
Good > CI PHPUnit tests are now failing after ed226e8
Test are failing as only one of the two suggested places for the adding $suffix
in the test expectations has been adjusted. If you add the second one, it should be okay AFAICS.
hellofromtonya commented on PR #3340:
2 years ago
#33
🤦♀️ Good catch @jrfnl Duh on my part. Fixed. Rebased to current trunk
. Pushed.
2 years ago
#34
Darn, still failing - I feared this may happen as the test are a "special kind of beast" and basing the suffix on SCRIPT_DEBUG
, like is done in wp_scripts_get_suffix()
was probably the wrong choice (though seemed like a logical one due to the call to wp_scripts_get_suffix()
already being there in the test function, even though unused).
In that case... I would suggest changing the suffix determination to be based on ABSPATH
.
Reason being:
- We don't want to duplicate logic used in the function under test in the test itself as then we wouldn't be testing anything anymore.
ABSPATH
in a test environment is defined in thewp-test-config.php
and will in "normal" (local) test situations end with/src/
and in Docker with/build/
(= CI).
By checking the value of
ABSPATH
, we should be able to figure out whether we should use.min
( for/build/
) or an empty suffix (for/src/
).
So something along the lines of (pseudo-code):
{{{diff
- $suffix = wp_scripts_get_suffix();
+ $suffix = str_ends_with(ABSPATH, '/src/') ? : WP_ASSET_SUFFIX_MIN;
}}}
2 years ago
#35
Note, the above is based on an "if I remember correctly" and will need to be verified.
hellofromtonya commented on PR #3340:
2 years ago
#36
Well lookie lookie @jrfnl's pseudo-code resolves both the CI and local testing issues. Cool. 👏 In review, it makes sense for the reasons she specified.
Tests now pass locally when:
npm run test:php
all tests ✅npm run test:php -- --filter
test_wp_add_inline_script_before_after_concat_with_core_dependency` ✅npm run test:php -- --group dependencies
✅
hellofromtonya commented on PR #3340:
2 years ago
#37
CI tests still failing. Investigating.
Yes, the wp-tests-config-sample.php
file defines ABSPATH
to be /src/
https://github.com/WordPress/wordpress-develop/blob/trunk/wp-tests-config-sample.php#L4
{{{php
define( 'ABSPATH', dirname( FILE ) . '/src/' );
}}}
That sample files is saved as wp-tests-config.php
here https://github.com/WordPress/wordpress-develop/blob/trunk/tools/local-env/scripts/install.js#L26-L33.
2 years ago
#38
In that case, we may as well just change the output expectation to use a regex and be done with it.
Or - looking at the code of that particular test now - change the preg_replace()
which is applied to the _actual_ output ($print_scripts
) to also replace .min.js
with .js
(and revert the changes in $expected
).
To be honest, looking at the test function, I'm not sure what it doesn't properly use expectOutputRegex()
in the first place....
#39
@
2 years ago
- Milestone changed from 6.1 to 6.2
With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone.
Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.
@hellofromTonya commented on PR #3340:
2 years ago
#40
As I'm out sick, I've pinged @azaozz to commit it.
2 years ago
#41
As I'm out sick, I've pinged @azaozz
This PR introduces changes to script-loader and default-constants, it is not a "pure" test PR. Don't see a compelling reason to commit it now, probably better after 6.1 is released.
Also I'm not sure what's the purpose of the new WP_ASSET_SUFFIX_MIN
constant and how it is supposed to be used.
2 years ago
#42
ABSPATH in a test environment is defined in the wp-test-config.php and will in "normal" (local) test situations end with /src/ and in Docker with /build/ (= CI).
That's not necessarily true. Think a lot of the wp-tests-config.php
files still contain code like:
/* Path to the WordPress codebase you'd like to test. Add a forward slash in the end. */ if ( defined( 'WP_RUN_CORE_TESTS' ) && WP_RUN_CORE_TESTS ) { define( 'ABSPATH', dirname( __FILE__ ) . '/build/' ); } else { define( 'ABSPATH', dirname( __FILE__ ) . '/src/' ); }
By checking the value of ABSPATH, we should be able to figure out whether we should use .min ( for /build/) or an empty suffix (for /src/).
That would mostly work. Will probably fail in places where the WP build repo is cloned. The path there will probably not have /build/
in it.
This ticket was mentioned in Slack in #core-committers by azaozz. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by costdev. View the logs.
2 years ago
#45
@
2 years ago
- Keywords changes-requested added
- Milestone changed from 6.2 to 6.3
The follow-up PR has changes requested and the tests are failing. Given that the PR touches source code (not just tests or build tools), moving this to 6.3 as 6.2 RC1 is next week and the last scheduled beta already happened.
The PR is close to done. A few tweaks and some follow-up testing it should be ready for committing in 6.3 when done.
2 years ago
#46
Coming back to this, I'm most likely missing something but what is the problem with having both
/assets/script-loader-packages.php /assets/script-loader-packages.min.php
(committed) in /src
and loading the "development" or "production" version according to whether SCRIPT_DEBUG
is set? Then can copy only /assets/script-loader-packages.min.php
to /build
if the non-minified scripts are never going to be useful in production, to save some space.
This is the usual way to define and use JS files in script-loader, why not use it here? That will also make WP_ASSET_SUFFIX_MIN
pointless, and generally simplify it a little.
#47
@
19 months ago
- Milestone changed from 6.3 to 6.4
Moving to milestone 6.4 as WP 6.3 RC3 has been released.
This ticket was mentioned in Slack in #core-test by sergey. View the logs.
19 months ago
This ticket was mentioned in Slack in #core by marybaum. View the logs.
17 months ago
#51
@
17 months ago
We the bug-scrubbers of October will leave yall to this! @hellofromTonya confirms build/test is not bound by release milestones.
#52
@
16 months ago
- Milestone changed from 6.4 to 6.5
Moving to milestone 6.5 as WP 6.4 RC3 has been released.
#53
@
12 months ago
- Keywords needs-patch added; has-patch changes-requested removed
- Milestone changed from 6.5 to 6.6
The two attached PRs here were closed and there's no traction; not clear to me what's still missing but clearly it doesn't look like something that's being worked on in 6.5
This ticket was mentioned in PR #6960 on WordPress/wordpress-develop by @desrosj.
8 months ago
#54
- Keywords has-patch added; needs-patch removed
Trac ticket:
In 54277: