Skip to content

Consolidate onBuild tests + verify imagesPath for multiple operating systems #101

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 4, 2023
Merged

Consolidate onBuild tests + verify imagesPath for multiple operating systems #101

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f352858
Consolidate tests and verify imagesPath containing path.posix.sep and…
gshel Nov 15, 2023
8b53602
Replace Windows file separator with posix separator for all imagesPat…
gshel Nov 16, 2023
29c96a0
Demonstrate bug where string imagesPath input is ignored, uses /image…
gshel Nov 16, 2023
7d9574f
Allow the os separator tested to be included in the test's title
gshel Nov 16, 2023
0d8caf5
Be super explicit about which separator to split on
gshel Nov 16, 2023
3b80792
Get input imagesPath from netlify.toml regardless of whether it is a …
gshel Nov 16, 2023
edf1a89
Add test to verify that imagesPath will default to `/images` if input…
gshel Nov 16, 2023
ed164ab
Change imagesPath default value to use CLOUDINARY_ASSET_DIRECTORIES
gshel Dec 1, 2023
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
5 changes: 2 additions & 3 deletions netlify-plugin-cloudinary/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,7 @@ export async function onBuild({
cname,
deliveryType,
folder = process.env.SITE_NAME || '',
imagesPath = CLOUDINARY_ASSET_DIRECTORIES.find(
({ inputKey }) => inputKey === 'imagesPath',
)?.path,
imagesPath = inputs.imagesPath || '/images',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at this again - the intent for using CLOUDINARY_ASSET_DIRECTORIES was so that i could define the different configurations for different asset types in one location

for instance, in the future there could be a second object in CLOUDINARY_ASSET_DIRECTORIES with a name of videos

im kind of surprised the default wasn't working properly as the code seems to run fine

image

can we keep the default value but use the inputs first if available like you have?
Copy link
Contributor Author

@gshel gshel Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heya!

im kind of surprised the default wasn't working properly as the code seems to run fine

I was surprised also, but I noticed the bug when testing locally and wrote two tests to confirm it.

To reproduce on the main branch:

  1. ✅ Run all tests successfully 
    pnpm -r test
  2. ✏️ Change line 129 of on-build.test.js to a string that is not /images 
    const imagesPath = '/notimages'
  3. ❌ Run all tests again; the test should create redirects with defaut fetch-based configuration in deploy preview context will fail.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep the default value but use the inputs first if available like you have?

I'll look into this now~
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅
maxSize,
privateCdn,
uploadPreset,
Expand Down Expand Up @@ -277,6 +275,7 @@ export async function onBuild({
}

mediaPaths.forEach(async mediaPath => {
mediaPath = mediaPath.split(path.win32.sep).join(path.posix.sep);
const cldAssetPath = `/${path.posix.join(PUBLIC_ASSET_PATH, mediaPath)}`;
const cldAssetUrl = `${host}${cldAssetPath}`;
try {
Expand Down
271 changes: 119 additions & 152 deletions netlify-plugin-cloudinary/tests/on-build.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,20 @@
import { vi, expect, describe, test, beforeEach,afterEach } from 'vitest';
import { promises as fs } from 'fs';
import path from 'node:path';
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
import { onBuild } from '../src/';
import { ERROR_API_CREDENTIALS_REQUIRED } from '../src/data/errors';

const contexts = [
{
name: 'production',
url: 'https://netlify-plugin-cloudinary.netlify.app'
},
{
name: 'deploy-preview',
url: 'https://deploy-preview-1234--netlify-plugin-cloudinary.netlify.app'
}
]

describe('onBuild', () => {
const ENV_ORIGINAL = process.env;
const readdir = fs.readdir;
Expand Down Expand Up @@ -51,7 +63,7 @@ describe('onBuild', () => {
},
utils: {
build: {
failBuild: () => {}
failBuild: () => { }
}
}
});
Expand All @@ -63,174 +75,129 @@ describe('onBuild', () => {

describe('Redirects', () => {

test('should create redirects with defaut fetch-based configuration in production context', async () => {
const imagesFunctionName = 'cld_images';
let deliveryType = 'fetch';
let netlifyConfig;
let redirects;

fs.readdir.mockResolvedValue([imagesFunctionName]);

process.env.CONTEXT = 'production';
process.env.URL = 'https://netlify-plugin-cloudinary.netlify.app';
const defaultRedirect = {
from: '/path',
to: '/other-path',
status: 200
}

beforeEach(async () => {
// Tests to ensure that delivery type of fetch works without API Key and Secret as it should

delete process.env.CLOUDINARY_API_KEY;
delete process.env.CLOUDINARY_API_SECRET;

const deliveryType = 'fetch';
const imagesPath = '/images';
// resets vars for each tests
redirects = [defaultRedirect];

const defaultRedirect = {
from: '/path',
to: '/other-path',
status: 200
}

const redirects = [defaultRedirect];

const netlifyConfig = {
netlifyConfig = {
redirects
};

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: `.next/out${imagesPath}`
},
inputs: {
deliveryType
}
});
expect(redirects[0]).toEqual({
from: `${imagesPath}/*`,
to: `https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/${deliveryType}/f_auto,q_auto/${process.env.URL}/cld-assets${imagesPath}/:splat`,
status: 302,
force: true
});

expect(redirects[1]).toEqual({
from: `/cld-assets${imagesPath}/*`,
to: `${imagesPath}/:splat`,
status: 200,
force: true
});

expect(redirects[2]).toEqual(defaultRedirect);
});

test('should create redirects with defaut fetch-based configuration in deploy preview context', async () => {
const imagesFunctionName = 'cld_images';

fs.readdir.mockResolvedValue([imagesFunctionName]);

process.env.CONTEXT = 'deploy-preview';
process.env.DEPLOY_PRIME_URL = 'https://deploy-preview-1234--netlify-plugin-cloudinary.netlify.app';

const deliveryType = 'fetch';
const imagesPath = '/images';

const defaultRedirect = {
from: '/path',
to: '/other-path',
status: 200
}

const redirects = [defaultRedirect];

const netlifyConfig = {
redirects
};

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: `.next/out${imagesPath}`
},
inputs: {
deliveryType
}
});

expect(redirects[0]).toEqual({
from: `${imagesPath}/*`,
to: `https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/${deliveryType}/f_auto,q_auto/${process.env.DEPLOY_PRIME_URL}/cld-assets${imagesPath}/:splat`,
status: 302,
force: true
});

expect(redirects[1]).toEqual({
from: `/cld-assets${imagesPath}/*`,
to: `${imagesPath}/:splat`,
status: 200,
force: true
});

expect(redirects[2]).toEqual(defaultRedirect);
});

test('should create redirects with multiple image paths', async () => {
const imagesFunctionName = 'cld_images';

fs.readdir.mockResolvedValue([imagesFunctionName]);
})

process.env.CONTEXT = 'deploy-preview';
process.env.DEPLOY_PRIME_URL = 'https://deploy-preview-1234--netlify-plugin-cloudinary.netlify.app';

const deliveryType = 'fetch';
const imagesPath = [ '/images', '/assets' ];

const defaultRedirect = {
from: '/path',
to: '/other-path',
status: 200
}

const redirects = [defaultRedirect];

const netlifyConfig = {
redirects
function validate(imagesPath, redirects, url) {
if (typeof (imagesPath) === 'string') {
imagesPath = [imagesPath]
};

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: __dirname
},
inputs: {
deliveryType,
imagesPath
}
let count = 0
imagesPath?.reverse().forEach(element => {
let i = element.split(path.win32.sep).join(path.posix.sep).replace('/', '');

expect(redirects[count]).toEqual({
from: `/${i}/*`,
to: `https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/${deliveryType}/f_auto,q_auto/${process.env.URL}/cld-assets/${i}/:splat`,
status: 302,
force: true
});

expect(redirects[count + 1]).toEqual({
from: `/cld-assets/${i}/*`,
to: `/${i}/:splat`,
status: 200,
force: true
});
count += 2;
});

expect(redirects[0]).toEqual({
from: `${imagesPath[1]}/*`,
to: `https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/${deliveryType}/f_auto,q_auto/${process.env.DEPLOY_PRIME_URL}/cld-assets${imagesPath[1]}/:splat`,
status: 302,
force: true
expect(redirects[redirects.length - 1]).toEqual(defaultRedirect);
}

describe.each(['posix', 'win32'])('Operating system: %o', (os) => {
let separator = path[os].sep;
let imagesPathStrings = [
'/images',
'/nest',
'/images/nesttest'
];
imagesPathStrings = imagesPathStrings.map(i => i.replace(/\//g, separator));

let imagesPathLists = [
[['/images', '/assets']],
[['/images/nest1', '/assets/nest2']],
[['/example/hey', '/mixed', '/test']]
];
imagesPathLists = imagesPathLists.map(collection =>
collection.map(imagesPath =>
imagesPath.map(i => i.replace(/\//g, separator))
)
);

describe.each(contexts)(`should create redirects with default ${deliveryType}-based configuration in $name context`, async (context) => {
process.env.URL = context.url;

test.each(imagesPathLists)('%o', async (imagesPath) => {

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: __dirname
},
inputs: {
deliveryType,
imagesPath
}
});
validate(imagesPath, redirects);
});

test.each(imagesPathStrings)('%o', async (imagesPath) => {

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: `.next/out${imagesPath}`
},
inputs: {
deliveryType,
imagesPath
}
});
validate(imagesPath, redirects);
});

test('imagesPath undefined', async () => {

await onBuild({
netlifyConfig,
constants: {
PUBLISH_DIR: '.next/out/'
},
inputs: {
deliveryType
}
});
let imagesPath = '/images';
validate(imagesPath, redirects);
});
});

expect(redirects[1]).toEqual({
from: `/cld-assets${imagesPath[1]}/*`,
to: `${imagesPath[1]}/:splat`,
status: 200,
force: true
});
expect(redirects[2]).toEqual({
from: `${imagesPath[0]}/*`,
to: `https://res.cloudinary.com/${process.env.CLOUDINARY_CLOUD_NAME}/image/${deliveryType}/f_auto,q_auto/${process.env.DEPLOY_PRIME_URL}/cld-assets${imagesPath[0]}/:splat`,
status: 302,
force: true
});

expect(redirects[3]).toEqual({
from: `/cld-assets${imagesPath[0]}/*`,
to: `${imagesPath[0]}/:splat`,
status: 200,
force: true
});

expect(redirects[4]).toEqual(defaultRedirect);
});

})
});

});