Consolidate onBuild tests + verify imagesPath for multiple operating systems

[gshel]

Description

Consolidated and added new tests to demonstrate bug where Windows imagesPath contains url-encoded \, caused by path.join() using os specific separator which is escaped \ for windows

Issue Ticket Number

#100

Fixes #100

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist

• I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
• I have created an issue ticket for this PR
• I have checked to ensure there aren't other open Pull Requests for the same update/change?
• I have performed a self-review of my own code
• I have run tests locally to ensure they all pass
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes needed to the documentation

[netlify]

✅ Deploy Preview for netlify-plugin-cloudinary ready!

Name	Link
🔨 Latest commit	ed164ab
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-cloudinary/deploys/656a4ffb2b02740008a5ed00
😎 Deploy Preview	https://deploy-preview-101--netlify-plugin-cloudinary.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[colbyfayock]

hey @gshel changes generally look good! looks like it's running into a little issue though

are you able to see the logs? https://github.com/cloudinary-community/netlify-plugin-cloudinary/actions/runs/6885078686/job/18751733492?pr=101

[gshel]

hey @gshel changes generally look good! looks like it's running into a little issue though

are you able to see the logs? https://github.com/cloudinary-community/netlify-plugin-cloudinary/actions/runs/6885078686/job/18751733492?pr=101

I can, thanks @colbyfayock! Looks like I've got some work to do on the win32 stuff. Is it possible that the tests were run on my first commit? I was trying to demonstrate that there were issues but a later commit included the fix. Here are my results locally:

I see that there is a posix test failing when imagesPath = '/images/nest'. I did some testing locally and it looks like if imagesPath is passed in as a string, it will always end up as /images regardless of the user input. Is this expected?

[colbyfayock]

definitely not expected! that may be a regression then? as it did work at one point. i dont currently have a test for that it seems

[gshel]

I crack myself up. I wrote tests to catch issues caused by using path.sep and then proceeded to use path.sep IN the tests 😶‍🌫️

[gshel]

Okey doke I think it should be ready to pass all the tests now 🤞

I fixed the issue where imagesPath would ignore the user input['imagesPath'] if it was a string, and instead always use /images

I also added tests to make sure that imagesPath will:

1. default to /images if input.imagesPath is undefined.
   e.g. input['imagesPath'] === undefined
   -> imagesPath = '/images'
2. use full value of input.imagesPath if it is defined as a string
   e.g. input['imagesPath'] === '/babygoats/here'
   -> imagesPath = '/babygoats/here'

There's quite a bit in this one PR; if you'd prefer I'd be happy to split it into separate PRs.

Thanks @colbyfayock !

[colbyfayock]

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

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

[gshel]

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.

[gshel]

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

I'll look into this now~

[gshel]

✅

[colbyfayock]

just one more question on the default value update!

[colbyfayock]

thanks @gshel!