#53 properly test next-on-netlify in onBuild #65

lindsaylevine · 2020-11-25T07:12:30Z

Fixes #53

lindsaylevine · 2020-11-25T07:18:19Z

thought: we could maybe try to get a dist from /sample

ehmicky · 2020-11-25T13:38:53Z

test/index.js

@@ -32,12 +32,22 @@ const changeCwd = function (cwd) {
  return process.chdir.bind(process, originalCwd)
 }

+// Move .next from sample project to current directory
+const moveNextDist = function () {
+  // Use copySync because cpx doesn't copy hidden files


Do you mean dotfiles by "hidden files"?
If so, I believe cpx does support it (see this issue). The problem might come from the globbing pattern instead. Most globbing libraries will ignore dotfiles with * and ** unless some dotfile option is passed. However, .* will work (see this comment).

oh huh i was going off this one but gotchu

If that does not work, cpy could also be used.

ive tried several things to copy .next with cpx and none have worked for me, feel free to pull n try n prove me wrong !!!

Did it work with cpy instead?

The following commit fixes it with cpy: b099569
I haven't pushed it so you can review it first.

i was def using cpy wrong, the word dot literally isnt even included in the readme

thanks for fixing!!!!!

ahhhh i cherry-picked and your commit fails the tests. im gonna rebase it out for now; could you update your branch and make sure the tests pass there and PR it into this branch?

dot is part of globby which is mentioned there :)

I fixed the bug and pushed to this PR in 7aa933c 👍

test/index.js

.github/workflows/test.yml

lindsaylevine · 2020-11-27T09:29:33Z

re: the conversation around the sample project/fixture - i hear what youre saying but dont see nearly a big enough issue to justify why certain files need to be removed. next-on-netlify has a ton of sample projects embedded in its cypress and test directories, with respective package.json filees and .gitignore files. when /sample was on the root level of the project, it existed with these same files (package.json required to, well, run the project to test the plugin as a local plugin and .gitignore to avoid committing files relevant to that project). if you'd really like, we can combine the .gitignore with the top-level, but because the sample project requires different dependencies to run as a local plugin, i don't think it makes sense to combine package.json files. i hope this makes sense! the one thing i will say is that sample doesnt really belong with the other fixtures and so i will move it up a level to /test/sample

ehmicky · 2020-11-30T15:12:16Z

What you say makes sense. However, when it comes to having multiple package.json, we do need to consider the following drawbacks:

This requires adding cd ... && npm install in the CI, which makes CI much slower. It also makes local development setup (since multiple directories node_modules would now need to be installed)
Dependencies upgrades would now need to be done in multiple package.json
At the moment, this is a single sample project. But new tests might add more of them, which would increase that maintenance cost.

Separating which dependencies are needed in specific samples/fixtures from the main dependencies is nice, but it feels to me that benefit would be outweighed by those drawbacks. Just adding samples/fixtures to the top-level package.json devDependencies would just work without any of those drawbacks. What do you think?

lindsaylevine · 2020-12-01T10:57:25Z

removing the package.json from the sample dir seems to interfere with testing the local plugin within the /sample project, which is and has been its primary purpose (see TESTING.md from original commit of the sample dir). we cant test with netlify cli without a package.json and the standard Next.js essentials. this is essential to plugin development and so unless you have a way to run netlify build and netlify deploy from sample without a package.json and the files you claim to be unnecessary, i'm going to assert the need to keep things as is.

lindsaylevine · 2020-12-01T11:10:47Z

separate issue: the plugin won't install as a local plugin with the tests, it seems. it complains about the jest keywords being undefined. unclear at this moment if this is just local plugin specific or an issue with installing the plugin in prod as well (that wouldn't make sense, though, since we have tests in main/published). what's the fix here for local plugin installation? not sure if it's obvious or a plugin issue, happy to open an issue as well.

ehmicky · 2020-12-01T12:24:43Z

not sure if it's obvious or a plugin issue, happy to open an issue as well.

Yes, that might be good (including build logs and steps to reproduce), thanks!

lindsaylevine · 2020-12-02T11:05:57Z

i think this should be good now!!!!!!!!! @ehmicky

ehmicky · 2020-12-02T15:31:25Z

test/sample/plugins/netlify-plugin-nextjs/index.js

@@ -0,0 +1,2 @@
+const plugin = require('../../../..')
+module.exports = plugin


This file can be removed thanks to netlify.toml using ../.. directly.

ehmicky

🎉 🎉 🎉

(Note: one small comment about a file which can be removed)

lindsaylevine requested a review from ehmicky November 25, 2020 07:12

lindsaylevine added the type: chore work needed to keep the product and development running smoothly label Nov 25, 2020

lindsaylevine force-pushed the ll/test-non branch from 7252626 to 4d765e5 Compare November 25, 2020 07:14

use .next dist to properly test next-on-netlify

7b17151

lindsaylevine force-pushed the ll/test-non branch from 4d765e5 to 7b17151 Compare November 25, 2020 07:16

lindsaylevine force-pushed the ll/test-non branch from 7c4be8c to dda8321 Compare November 25, 2020 11:07

lindsaylevine marked this pull request as draft November 25, 2020 11:10

ehmicky reviewed Nov 25, 2020

View reviewed changes

test/index.js Outdated Show resolved Hide resolved

ehmicky reviewed Nov 25, 2020

View reviewed changes

test/index.js Outdated Show resolved Hide resolved

ehmicky reviewed Nov 25, 2020

View reviewed changes

test/index.js Outdated Show resolved Hide resolved

ehmicky reviewed Nov 25, 2020

View reviewed changes

test/index.js Outdated Show resolved Hide resolved

lindsaylevine force-pushed the ll/test-non branch 11 times, most recently from 8952673 to 37ec572 Compare November 27, 2020 09:23

lindsaylevine commented Nov 27, 2020

View reviewed changes

.github/workflows/test.yml Outdated Show resolved Hide resolved

update next-on-netlify testing to use sample/.next dist

e5b5980

lindsaylevine force-pushed the ll/test-non branch from 37ec572 to e5b5980 Compare November 27, 2020 09:26

lindsaylevine marked this pull request as ready for review November 27, 2020 09:26

lindsaylevine force-pushed the ll/test-non branch from 5361528 to 89a8bad Compare November 27, 2020 09:44

move /test/fixtures/sample to /test/sample

9fff034

lindsaylevine force-pushed the ll/test-non branch from 89a8bad to 9fff034 Compare November 27, 2020 09:48

lindsaylevine force-pushed the ll/test-non branch from 25575cf to 9fff034 Compare December 1, 2020 11:06

Fix files copy

7aa933c

Fix sample directory and delete files

cb29ba6

lindsaylevine force-pushed the ll/test-non branch from c92daba to cb29ba6 Compare December 2, 2020 10:56

remove unnecessary items

917c318

ehmicky reviewed Dec 2, 2020

View reviewed changes

ehmicky approved these changes Dec 2, 2020

View reviewed changes

remove plugins dir in test/sample

be45c69

lindsaylevine merged commit 9da8be5 into main Dec 2, 2020

lindsaylevine deleted the ll/test-non branch December 2, 2020 17:51

serhalp pushed a commit that referenced this pull request Apr 5, 2024

ci: add easy way to create pre releases (#65)

bee85d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#53 properly test next-on-netlify in onBuild #65

#53 properly test next-on-netlify in onBuild #65

lindsaylevine commented Nov 25, 2020 •

edited

Loading

lindsaylevine commented Nov 25, 2020

ehmicky Nov 25, 2020

lindsaylevine Nov 25, 2020

ehmicky Nov 25, 2020

lindsaylevine Nov 26, 2020

ehmicky Nov 26, 2020

lindsaylevine Nov 27, 2020

ehmicky Nov 30, 2020

lindsaylevine Dec 1, 2020

lindsaylevine Dec 1, 2020 •

edited

Loading

ehmicky Dec 1, 2020 •

edited

Loading

lindsaylevine commented Nov 27, 2020 •

edited

Loading

ehmicky commented Nov 30, 2020

lindsaylevine commented Dec 1, 2020

lindsaylevine commented Dec 1, 2020

ehmicky commented Dec 1, 2020

lindsaylevine commented Dec 2, 2020

ehmicky Dec 2, 2020

ehmicky left a comment

		@@ -0,0 +1,2 @@
		const plugin = require('../../../..')
		module.exports = plugin

#53 properly test next-on-netlify in onBuild #65

#53 properly test next-on-netlify in onBuild #65

Conversation

lindsaylevine commented Nov 25, 2020 • edited Loading

lindsaylevine commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindsaylevine Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

ehmicky Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

lindsaylevine commented Nov 27, 2020 • edited Loading

ehmicky commented Nov 30, 2020

lindsaylevine commented Dec 1, 2020

lindsaylevine commented Dec 1, 2020

ehmicky commented Dec 1, 2020

lindsaylevine commented Dec 2, 2020

Choose a reason for hiding this comment

ehmicky left a comment

Choose a reason for hiding this comment

lindsaylevine commented Nov 25, 2020 •

edited

Loading

lindsaylevine Dec 1, 2020 •

edited

Loading

ehmicky Dec 1, 2020 •

edited

Loading

lindsaylevine commented Nov 27, 2020 •

edited

Loading