Repository navigation
env: fall back to last known WordPress version if the mirror hasn't synced the new tag yet #81534
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
Open
josephfusco
wants to merge
11
commits into
WordPress:trunk
Choose a base branch
from
josephfusco:fix/wp-env-latest-tag-fallback
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
89b0cd0
env: fall back to last known WordPress version if the git mirror hasn…
josephfusco 1c547f7
env: fall back to the mirror's own highest tag when there is no cache…
josephfusco 606fdb3
env: avoid mutating the mirror-versions array, sharpen a test assertion
josephfusco 21a286f
env: document why the version fallback can't surface a warning
josephfusco 88606e5
env: tighten fallback comments for clarity
josephfusco df691b5
Update packages/env/lib/wordpress.js
josephfusco d20c5bf
Update packages/env/lib/wordpress.js
josephfusco 03738e2
Update packages/env/lib/wordpress.js
josephfusco 0a7e3a2
env: warn on fallback, drop unclear cache read, fix null-check after …
josephfusco 48b2a36
Merge branch 'trunk' into fix/wp-env-latest-tag-fallback
josephfusco 33447bd
Merge branch 'trunk' into fix/wp-env-latest-tag-fallback
mcsf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| 'use strict'; | ||
|
|
||
| jest.mock( 'dns', () => ( { | ||
| promises: { | ||
| resolve: jest.fn(), | ||
| }, | ||
| } ) ); | ||
| jest.mock( 'got', () => jest.fn() ); | ||
| jest.mock( 'simple-git', () => jest.fn() ); | ||
| jest.mock( '../cache', () => ( { | ||
| getCache: jest.fn(), | ||
| setCache: jest.fn(), | ||
| } ) ); | ||
|
|
||
| const cacheOptions = { cacheDirectoryPath: '/a/b/c' }; | ||
|
|
||
| function mockStableCheck( got, version ) { | ||
| got.mockReturnValue( { | ||
| json: () => | ||
| Promise.resolve( { | ||
| [ version ]: 'latest', | ||
| } ), | ||
| } ); | ||
| } | ||
|
|
||
| function mockMirrorTags( SimpleGit, tags ) { | ||
| SimpleGit.mockReturnValue( { | ||
| listRemote: () => | ||
| Promise.resolve( | ||
| tags.map( ( tag ) => `abc\trefs/tags/${ tag }` ).join( '\n' ) | ||
| ), | ||
| } ); | ||
| } | ||
|
|
||
| // `getLatestWordPressVersion` memoizes its result in a module-level variable, | ||
| // so each test resets the module registry and re-requires its dependencies | ||
| // fresh to get an unmemoized copy with its own mock state. | ||
| describe( 'getLatestWordPressVersion', () => { | ||
| beforeEach( () => { | ||
| jest.resetModules(); | ||
| } ); | ||
|
|
||
| it( 'returns the version reported as latest when the mirror already has the tag', async () => { | ||
| const dns = require( 'dns' ).promises; | ||
| const got = require( 'got' ); | ||
| const SimpleGit = require( 'simple-git' ); | ||
| const { setCache } = require( '../cache' ); | ||
| const { getLatestWordPressVersion } = require( '../wordpress' ); | ||
|
|
||
| dns.resolve.mockResolvedValue( [ '127.0.0.1' ] ); | ||
| mockStableCheck( got, '6.6.0' ); | ||
| mockMirrorTags( SimpleGit, [ '6.5.9', '6.6.0' ] ); | ||
|
|
||
| const version = await getLatestWordPressVersion( cacheOptions ); | ||
|
|
||
| expect( version ).toBe( '6.6.0' ); | ||
| expect( setCache ).toHaveBeenCalledWith( | ||
| 'latestWordPressVersion', | ||
| '6.6.0', | ||
| expect.anything() | ||
| ); | ||
| } ); | ||
|
|
||
| it( 'falls back to the highest version on the mirror when it has not synced the new tag yet, and warns about it', async () => { | ||
| const dns = require( 'dns' ).promises; | ||
| const got = require( 'got' ); | ||
| const SimpleGit = require( 'simple-git' ); | ||
| const { setCache } = require( '../cache' ); | ||
| const { getLatestWordPressVersion } = require( '../wordpress' ); | ||
|
|
||
| dns.resolve.mockResolvedValue( [ '127.0.0.1' ] ); | ||
| mockStableCheck( got, '6.6.0' ); | ||
| mockMirrorTags( SimpleGit, [ | ||
| '6.5.8', | ||
| '6.5.10', | ||
| '6.5.9', | ||
| 'not-a-version', | ||
| ] ); | ||
|
|
||
| const version = await getLatestWordPressVersion( cacheOptions ); | ||
|
|
||
| expect( version ).toBe( '6.5.10' ); | ||
| expect( setCache ).toHaveBeenCalledWith( | ||
| 'latestWordPressVersion', | ||
| '6.5.10', | ||
| expect.anything() | ||
| ); | ||
| expect( console ).toHaveWarned(); | ||
| } ); | ||
|
|
||
| it( 'uses the reported version anyway when the mirror listing itself fails', async () => { | ||
| const dns = require( 'dns' ).promises; | ||
| const got = require( 'got' ); | ||
| const SimpleGit = require( 'simple-git' ); | ||
| const { getLatestWordPressVersion } = require( '../wordpress' ); | ||
|
|
||
| dns.resolve.mockResolvedValue( [ '127.0.0.1' ] ); | ||
| mockStableCheck( got, '6.6.0' ); | ||
| SimpleGit.mockReturnValue( { | ||
| listRemote: () => Promise.reject( new Error( 'network error' ) ), | ||
| } ); | ||
|
|
||
| const version = await getLatestWordPressVersion( cacheOptions ); | ||
|
|
||
| expect( version ).toBe( '6.6.0' ); | ||
| } ); | ||
| } ); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,11 @@ const dns = require( 'dns' ).promises; | |
| const fs = require( 'fs' ).promises; | ||
| const path = require( 'path' ); | ||
| const got = require( 'got' ); | ||
| const SimpleGit = require( 'simple-git' ); | ||
| const { getCache, setCache } = require( './cache' ); | ||
|
|
||
| const WORDPRESS_MIRROR_URL = 'https://github.com/WordPress/WordPress.git'; | ||
|
|
||
| /** | ||
| * @typedef {import('./config').WPSource} WPSource | ||
| */ | ||
|
|
@@ -58,8 +61,58 @@ async function canAccessWPORG() { | |
| } | ||
|
|
||
| /** | ||
| * Returns the latest stable version of WordPress by requesting the stable-check | ||
| * endpoint on WordPress.org. | ||
| * Returns every stable version tag currently on the WordPress/WordPress | ||
| * mirror, which is what wp-env actually fetches WordPress core from. | ||
| * | ||
| * @return {?string[]} Every available version, like [ '6.5.8', '6.5.9' ]. | ||
| * Null if the mirror couldn't be queried, e.g. the | ||
| * remote or the local network connection is down. | ||
| */ | ||
| async function getMirrorVersions() { | ||
| try { | ||
| const output = await SimpleGit().listRemote( [ | ||
| '--tags', | ||
| WORDPRESS_MIRROR_URL, | ||
| ] ); | ||
| return output | ||
| .split( '\n' ) | ||
| .map( | ||
| ( line ) => | ||
| line.match( /refs\/tags\/(\d+\.\d+(?:\.\d+)?)$/ )?.[ 1 ] | ||
| ) | ||
| .filter( Boolean ); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Compares two dotted-numeric WordPress version strings, like "6.5.9" and | ||
| * "6.5.10" or "6.6". | ||
| * | ||
| * @param {string} a A version string. | ||
| * @param {string} b Another version string. | ||
| * @return {number} Negative if a < b, positive if a > b, zero if equal. | ||
| */ | ||
| function compareWordPressVersions( a, b ) { | ||
| const partsA = a.split( '.' ).map( Number ); | ||
| const partsB = b.split( '.' ).map( Number ); | ||
|
|
||
| for ( let i = 0; i < Math.max( partsA.length, partsB.length ); i++ ) { | ||
| const diff = ( partsA[ i ] || 0 ) - ( partsB[ i ] || 0 ); | ||
| if ( diff !== 0 ) { | ||
| return diff; | ||
| } | ||
| } | ||
|
|
||
| return 0; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the latest stable version of WordPress, as reported by the | ||
| * stable-check endpoint on WordPress.org. If the WordPress/WordPress mirror | ||
| * hasn't synced that version's tag yet, falls back to a version the mirror | ||
| * actually has. | ||
| * | ||
| * @param {Object} options an object with cacheDirectoryPath set to the path to the cache directory in ~/.wp-env. | ||
| * @return {string} The latest stable version of WordPress, like "6.0.1" | ||
|
|
@@ -96,6 +149,37 @@ async function getLatestWordPressVersion( options ) { | |
|
|
||
| for ( const [ version, status ] of Object.entries( versions ) ) { | ||
| if ( status === 'latest' ) { | ||
| const mirrorVersions = await getMirrorVersions(); | ||
|
|
||
| // A null mirrorVersions means the listing itself failed, most | ||
| // likely a network issue -- that isn't confirmation the tag is | ||
| // missing, so fall through to using the reported version as | ||
| // before. | ||
| if ( | ||
| mirrorVersions !== null && | ||
| ! mirrorVersions.includes( version ) | ||
| ) { | ||
| // The mirror hasn't synced this tag yet, so fall back to the | ||
| // highest version it actually has instead of returning a | ||
| // version that's guaranteed to fail the `git fetch` that | ||
| // follows. | ||
| const fallback = [ ...mirrorVersions ] | ||
| .sort( compareWordPressVersions ) | ||
| .pop(); | ||
|
Comment on lines
+166
to
+168
Contributor
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. Let's nevertheless exclude const versions = await got(
'https://api.wordpress.org/core/stable-check/1.0/'
).json();
+const secureVersions = new Set(
+ versions.filter( version => /* exclude those marked "insecure" */ )
+);
const fallback = [ ...mirrorVersions ]
+ .filter( secureVersion.has )
.sort( compareWordPressVersions )
.pop(); |
||
|
|
||
| console.warn( | ||
| `wp-env: WordPress ${ version } hasn't synced to the WordPress/WordPress mirror yet. Using ${ fallback } instead.` | ||
| ); | ||
|
|
||
| CACHED_WP_VERSION = fallback; | ||
| await setCache( | ||
| 'latestWordPressVersion', | ||
| fallback, | ||
| cacheOptions | ||
| ); | ||
| return fallback; | ||
| } | ||
|
|
||
| CACHED_WP_VERSION = version; | ||
| await setCache( 'latestWordPressVersion', version, cacheOptions ); | ||
| return version; | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Mocking externals makes me anxious. I'd be more open to our implementing interfacing functions that we can then mock more confidently. For example:
Likewise, introducing
fetchStableCheck.I don't love it, but it seems like a lesser evil.
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.
Thanks for the feedback, appreciate the thorough review.
parse-config.jsandconfig-integration.jsalready mockgotandsimple-gitdirectly, in this same package. Wrapping only this file's calls would leave three different mocking approaches to the same two libraries inpackages/env, one more inconsistency, not less. If this is worth doing, it reads as a package-wide test convention change rather than something scoped to this PR.