From 2fa45f9acf0a759e5743799a747984279f442925 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Wed, 22 Feb 2023 13:19:04 -0500 Subject: [PATCH] end-to-end test link rewriting plugin (#34939) --- .github/workflows/test.yml | 14 ++ script/update-internal-links.js | 19 ++- .../content/get-started/quickstart/index.md | 1 + .../get-started/quickstart/link-rewriting.md | 30 ++++ .../page-with-deprecated-enterprise-links.md | 11 -- tests/rendering-fixtures/internal-links.js | 62 +++++++- tests/unit/page.js | 135 +----------------- 7 files changed, 118 insertions(+), 154 deletions(-) create mode 100644 tests/fixtures/content/get-started/quickstart/link-rewriting.md delete mode 100644 tests/fixtures/page-with-deprecated-enterprise-links.md diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 850e6c1a5a73..492c27ee658f 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -115,6 +115,20 @@ jobs: if: ${{ matrix.test-group == 'rendering-fixtures' }} run: ./script/copy-fixture-data.js --check + # This keeps our fixture content/data in check + - name: Check the test fixture content (if applicable) + if: ${{ matrix.test-group == 'rendering-fixtures' }} + env: + ROOT: tests/fixtures + run: | + # If either of these fail, it means our fixture content's internal + # links can and should be updated. + ./script/update-internal-links.js --dry-run --check --strict --verbose \ + tests/fixtures/content \ + --exclude tests/fixtures/content/get-started/foo/typo-autotitling.md + ./script/update-internal-links.js --dry-run --check --strict --verbose \ + tests/fixtures/data + - name: Clone all translations if: ${{ matrix.test-group == 'translations' }} uses: ./.github/actions/clone-translations diff --git a/script/update-internal-links.js b/script/update-internal-links.js index cb7c85c02216..2549a590a4fa 100755 --- a/script/update-internal-links.js +++ b/script/update-internal-links.js @@ -29,6 +29,7 @@ program .option('--check', 'Exit and fail if it found something to fix') .option('--aggregate-stats', 'Display aggregate numbers about all possible changes') .option('--strict', "Throw an error (instead of a warning) if a link can't be processed") + .option('--exclude [paths...]', 'Specific files to exclude') .arguments('[files-or-directories...]', '') .parse(process.argv) @@ -37,6 +38,8 @@ main(program.args, program.opts()) async function main(files, opts) { const { debug } = opts + const excludeFilePaths = new Set(opts.exclude || []) + try { if (opts.check && !opts.dryRun) { throw new Error("Can't use --check without --dry-run") @@ -47,15 +50,25 @@ async function main(files, opts) { files.push('content', 'data') } for (const file of files) { - if (!(file.startsWith('content') || file.startsWith('data'))) { + if ( + !( + file.startsWith('content') || + file.startsWith('data') || + file.startsWith('tests/fixtures') + ) + ) { throw new Error(`${file} must be a content or data filepath`) } if (!fs.existsSync(file)) { throw new Error(`${file} does not exist`) } if (fs.lstatSync(file).isDirectory()) { - actualFiles.push(...walkFiles(file, ['.md', '.yml'])) - } else { + actualFiles.push( + ...walkFiles(file, ['.md', '.yml']).filter((p) => { + return !excludeFilePaths.has(p) + }) + ) + } else if (!excludeFilePaths.has(file)) { actualFiles.push(file) } } diff --git a/tests/fixtures/content/get-started/quickstart/index.md b/tests/fixtures/content/get-started/quickstart/index.md index 40e9cb45b80a..6600b7c498c8 100644 --- a/tests/fixtures/content/get-started/quickstart/index.md +++ b/tests/fixtures/content/get-started/quickstart/index.md @@ -8,4 +8,5 @@ versions: ghec: '*' children: - /hello-world + - /link-rewriting --- diff --git a/tests/fixtures/content/get-started/quickstart/link-rewriting.md b/tests/fixtures/content/get-started/quickstart/link-rewriting.md new file mode 100644 index 000000000000..9bac58724f0a --- /dev/null +++ b/tests/fixtures/content/get-started/quickstart/link-rewriting.md @@ -0,0 +1,30 @@ +--- +title: Link rewriting +intro: 'When rendered, links are rewritting to perfection for viewing' +versions: + fpt: '*' + ghes: '*' + ghae: '*' + ghec: '*' +type: quick_start +--- + +## Internal links never need language prefix + +[AUTOTITLE](/get-started/foo/cross-version-linking) already tests things +like `/enterprise-server@latest/` becomes `/enterprise-server@X.Y/` where +`X.Y` is the latest Enterprise server version. + +## External links are left alone + +[@peterbe](https://github.com/peterbe) + +## Legacy enterprise links + +For example, [this Enterprise 11.10 link](/enterprise/11.10.340/admin/articles/upgrading-to-the-latest-release). + +## Links to `assets` and `public` + +Here's a [Picture](/assets/images/_fixtures/screenshot.png). + +[A GraphQL Schema](/public/schema.docs.graphql) diff --git a/tests/fixtures/page-with-deprecated-enterprise-links.md b/tests/fixtures/page-with-deprecated-enterprise-links.md deleted file mode 100644 index a2dc958c7d0f..000000000000 --- a/tests/fixtures/page-with-deprecated-enterprise-links.md +++ /dev/null @@ -1,11 +0,0 @@ ---- -title: Page with deprecated enterprise links -versions: - free-pro-team: '*' - enterprise-server: '*' ---- - - -- [Version 2.22](/enterprise-server@2.22) - -- [Version 3.2](/enterprise-server@3.2) diff --git a/tests/rendering-fixtures/internal-links.js b/tests/rendering-fixtures/internal-links.js index 7002c6b915b7..b6f87eb07b84 100644 --- a/tests/rendering-fixtures/internal-links.js +++ b/tests/rendering-fixtures/internal-links.js @@ -40,18 +40,68 @@ describe('cross-version-links', () => { const links = $('#article-contents a[href]') // Tests that the hardcoded prefix is always removed - const firstLink = links.filter(function () { - return $(this).text() === 'Hello world always in free-pro-team' - }) + const firstLink = links.filter( + (i, element) => $(element).text() === 'Hello world always in free-pro-team' + ) expect(firstLink.attr('href')).toBe('/en/get-started/quickstart/hello-world') // Tests that the second link always goes to enterprise-server@X.Y - const secondLink = links.filter(function () { - return $(this).text() === 'Autotitling page always in enterprise-server latest' - }) + const secondLink = links.filter( + (i, element) => $(element).text() === 'Autotitling page always in enterprise-server latest' + ) expect(secondLink.attr('href')).toBe( `/en/enterprise-server@${enterpriseServerReleases.latest}/get-started/quickstart/hello-world` ) } ) }) + +describe('link-rewriting', () => { + test('/en is injected', async () => { + const $ = await getDOM('/get-started/quickstart/link-rewriting') + const links = $('#article-contents a[href]') + + { + const link = links.filter((i, element) => $(element).text() === 'Cross Version Linking') + expect(link.attr('href')).toMatch('/en/get-started/') + } + + // Some links are left untouched + + { + const link = links.filter((i, element) => $(element).text().includes('Enterprise 11.10')) + expect(link.attr('href')).toMatch('/en/enterprise/') + } + { + const link = links.filter((i, element) => $(element).text().includes('peterbe')) + expect(link.attr('href')).toMatch(/^https:/) + } + { + const link = links.filter((i, element) => $(element).text().includes('Picture')) + expect(link.attr('href')).toMatch(/^\/assets\//) + } + { + const link = links.filter((i, element) => $(element).text().includes('GraphQL Schema')) + expect(link.attr('href')).toMatch(/^\/public\//) + } + }) + + test('/en and current version (latest) is injected', async () => { + const $ = await getDOM('/enterprise-cloud@latest/get-started/quickstart/link-rewriting') + const links = $('#article-contents a[href]') + + const link = links.filter((i, element) => $(element).text() === 'Cross Version Linking') + expect(link.attr('href')).toMatch('/en/enterprise-cloud@latest/get-started/') + }) + + test('/en and current version number is injected', async () => { + // enterprise-server, unlike enterprise-cloud, use numbers + const $ = await getDOM('/enterprise-server@latest/get-started/quickstart/link-rewriting') + const links = $('#article-contents a[href]') + + const link = links.filter((i, element) => $(element).text() === 'Cross Version Linking') + expect(link.attr('href')).toMatch( + `/en/enterprise-server@${enterpriseServerReleases.latest}/get-started/` + ) + }) +}) diff --git a/tests/unit/page.js b/tests/unit/page.js index 2e993c9ec81b..6bccf697e2b4 100644 --- a/tests/unit/page.js +++ b/tests/unit/page.js @@ -68,140 +68,7 @@ describe('Page class', () => { }) }) - describe.skip('page.render(context)', () => { - test('rewrites links to include the current language prefix and version', async () => { - const page = await Page.init(opts) - const context = { - page: { version: `enterprise-server@${enterpriseServerReleases.latest}` }, - currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`, - currentPath: - '/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches', - currentLanguage: 'en', - } - const rendered = await page.render(context) - const $ = cheerio.load(rendered) - expect( - page.markdown.includes( - '(/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests)' - ) - ).toBe(true) - expect( - page.markdown.includes( - '(/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests)' - ) - ).toBe(false) - expect( - $( - 'a[href="/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests"]' - ).length - ).toBe(0) - expect( - $( - `a[href="/en/${`enterprise-server@${enterpriseServerReleases.latest}`}/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests"]` - ).length - ).toBeGreaterThan(0) - }) - - // Much of this test is based on making sure we don't - // repeat the bug introduced in issue 1545. - test('rewrites links correctly for unsupported enterprise-server links', async () => { - const page = await Page.init({ - relativePath: 'page-with-deprecated-enterprise-links.md', - basePath: path.join(__dirname, '../fixtures'), - languageCode: 'en', - }) - const context = { - page: { version: `enterprise-server@${enterpriseServerReleases.latest}` }, - currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`, - currentPath: '/en/page-with-deprecated-enterprise-links', - currentLanguage: 'en', - } - const rendered = await page.render(context) - // That page only contains exactly 2 links. And we can know - // exactly what we expect each one to be. - const $ = cheerio.load(rendered) - const first = $('a[href]').first() - expect(first.text()).toBe('Version 2.22') - expect(first.attr('href')).toBe('/en/enterprise-server@2.22') - const last = $('a[href]').last() - expect(last.text()).toBe('Version 3.2') - expect(last.attr('href')).toBe('/en/enterprise-server@3.2') - }) - - test('rewrites links in the intro to include the current language prefix and version', async () => { - const page = await Page.init(opts) - page.rawIntro = - '[Pull requests](/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests)' - const context = { - page: { version: nonEnterpriseDefaultVersion }, - currentVersion: nonEnterpriseDefaultVersion, - currentPath: - '/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches', - currentLanguage: 'en', - } - // This is needed because unit tests are weird. The page.render() - // method is dependent on module global cache. - // We need to fudge the `currentPath` so it appears to be different. - context.currentPath += Math.random() - await page.render(context) - const $ = cheerio.load(page.intro) - expect( - $( - 'a[href="/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests"]' - ).length - ).toBe(0) - expect( - $( - 'a[href="/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-pull-requests"]' - ).length - ).toBeGreaterThan(0) - }) - - test('does not rewrite links that include deprecated enterprise release numbers', async () => { - const page = await Page.init({ - relativePath: - 'admin/enterprise-management/updating-the-virtual-machine-and-physical-resources/migrating-from-github-enterprise-1110x-to-2123.md', - basePath: path.join(__dirname, '../../content'), - languageCode: 'en', - }) - const context = { - page: { version: `enterprise-server@${enterpriseServerReleases.latest}` }, - currentVersion: `enterprise-server@${enterpriseServerReleases.latest}`, - currentPath: `/en/enterprise-server@${enterpriseServerReleases.latest}/admin/enterprise-management/migrating-from-github-enterprise-1110x-to-2123`, - currentLanguage: 'en', - } - const rendered = await page.render(context) - const $ = cheerio.load(rendered) - expect( - page.markdown.includes( - '(/enterprise/11.10.340/admin/articles/upgrading-to-the-latest-release/)' - ) - ).toBe(true) - expect( - $( - `a[href="/en/enterprise-server@${enterpriseServerReleases.latest}/11.10.340/admin/articles/upgrading-to-the-latest-release"]` - ).length - ).toBe(0) - expect( - $('a[href="/en/enterprise/11.10.340/admin/articles/upgrading-to-the-latest-release"]') - .length - ).toBeGreaterThan(0) - }) - - test('does not rewrite links to external redirects', async () => { - const page = await Page.init(opts) - page.markdown = `${page.markdown}\n\nSee [Capistrano](/capistrano).` - const context = { - page: { version: nonEnterpriseDefaultVersion }, - currentVersion: nonEnterpriseDefaultVersion, - currentPath: `/en/${nonEnterpriseDefaultVersion}/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches`, - currentLanguage: 'en', - } - const rendered = await page.render(context) - const $ = cheerio.load(rendered) - expect($('a[href="/capistrano"]').length).toBe(1) - }) - + describe('page.render(context)', () => { // Most of our Liquid versioning tests are in https://github.com/docs/render-content, // But they don't have access to our currently supported versions, which we're testing here. // This test ensures that this works as expected: {% if enterpriseServerVersions contains currentVersion %}