-
Notifications
You must be signed in to change notification settings - Fork 10
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
RFC: User Config file integration #53
Changes from 5 commits
76f9943
5bfd5d7
25a921f
88c95c1
654a80d
14e1f25
d349358
590ced0
bb7e22d
5d3f8c0
b53fe73
3053247
3cd95cb
7528cea
cd810fe
12133d3
1891719
10151c9
d7ead99
42fbc3a
9fce093
1a0c400
69d9ca4
7e195d8
0a542ac
ebb30ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
|
||
let config = { | ||
source: path.join(process.cwd(), 'src'), | ||
devServer: { | ||
port: 1984, | ||
host: 'localhost' | ||
}, | ||
publicPath: '/', | ||
// TODO add global meta data see issue #5 | ||
// https://github.com/ProjectEvergreen/greenwood/issues/5 | ||
meta: { | ||
title: '', | ||
description: '', | ||
author: '', | ||
domain: '' | ||
} | ||
}; | ||
|
||
module.exports = readAndMergeConfig = async() => { | ||
if (fs.existsSync(path.join(process.cwd(), 'greenwood.config.json'))) { | ||
const userCfgFile = require(path.join(process.cwd(), 'greenwood.config.json')); | ||
|
||
// prepend paths with current directory | ||
if (userCfgFile.source) { | ||
hutchgrant marked this conversation as resolved.
Show resolved
Hide resolved
|
||
userCfgFile.source = path.join(process.cwd(), userCfgFile.source); | ||
} | ||
|
||
config = { ...config, ...userCfgFile }; | ||
} | ||
return config; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,4 +160,67 @@ describe('building greenwood with error handling for app and page templates', () | |
await fs.remove(CONTEXT.scratchDir); | ||
}); | ||
|
||
}); | ||
|
||
describe('building greenwood with user provided config file', () => { | ||
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. if the intent is to also test with a nested directory output, lets add it to the In essence, we can try and have tests that take advantage of other tests already testing for specific outputs, so tests can have minimal overlap while still breaking each other if something goes wrong when making a change. 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. They aren't covered under this scenario, thats why they are there. In theory they should break earlier in the tests, but not necessarily with all the tests. Every single situation the public directory, js bundle, hello, nested, need to be tested. When the source directory changes(which it does as per this test when a config file is copied), everything has to be retested or at least partially. For example, had we tested the index route in the nested pages describe here(using user workspace mock-app), we would have caught the error for #54 which I just found today because one of the duplicate files which was in the correct path wasn't serialized. 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.
Right good point. And I think that's a good approach to take here, as for changing something like the One thing that isn't so clear here though, and maybe this could just be made more clear in a I would expect to see specific tests to cover each config API being supported (
That we call those out specifically in our testing somehow, like though
Although some cases may have some overlap, I think it's OK if it means that we can define and test the critical paths and outputs of a given feature / workflow / etc for the project if it ensures our tests can be as faithful to how a user would also setup / run our code. Providing a great and accurate development / testing environment we'll provide us much more grounded and confident in the usefulness and coverage of our tests. A little duplication for the sake of clarity and setup is a positive tradeoff / investment in my book 👍
|
||
before(async () => { | ||
setup = new TestSetup(); | ||
CONTEXT = await setup.init(); | ||
|
||
// read user config file and copy it to app root | ||
const userCfgFile = require(CONTEXT.userCfgPath); | ||
|
||
await fs.copy(CONTEXT.userCfgPath, CONTEXT.userCfgRootPath); | ||
|
||
// set new user source based on config file | ||
CONTEXT.userSrc = path.join(__dirname, '..', userCfgFile.source); | ||
|
||
// copy test app to configured source | ||
await fs.copy(CONTEXT.testApp, CONTEXT.userSrc); | ||
await setup.run(['./packages/cli/index.js', 'build']); | ||
|
||
blogPageHtmlPath = path.join(CONTEXT.publicDir, 'blog', '20190326', 'index.html'); | ||
}); | ||
|
||
it('should output one JS bundle', async() => { | ||
expect(await glob.promise(path.join(CONTEXT.publicDir, './**/index.*.bundle.js'))).to.have.lengthOf(1); | ||
}); | ||
|
||
it('should contain a nested blog page directory', () => { | ||
expect(fs.existsSync(path.join(CONTEXT.publicDir, 'blog', '20190326'))).to.be.true; | ||
}); | ||
|
||
describe('nested generated blog page directory', () => { | ||
const defaultHeading = 'Blog Page'; | ||
const defaultBody = 'This is the blog page built by Greenwood.'; | ||
let dom; | ||
|
||
beforeEach(async() => { | ||
dom = await JSDOM.fromFile(blogPageHtmlPath); | ||
}); | ||
|
||
it('should contain a nested blog page with an index html file', () => { | ||
expect(fs.existsSync(blogPageHtmlPath)).to.be.true; | ||
}); | ||
|
||
it('should have the expected heading text within the blog page in the blog directory', async() => { | ||
const heading = dom.window.document.querySelector('h3.wc-md-blog').textContent; | ||
|
||
expect(heading).to.equal(defaultHeading); | ||
}); | ||
|
||
it('should have the expected paragraph text within the blog page in the blog directory', async() => { | ||
let paragraph = dom.window.document.querySelector('p.wc-md-blog').textContent; | ||
|
||
expect(paragraph).to.equal(defaultBody); | ||
}); | ||
}); | ||
|
||
after(async() => { | ||
await fs.remove(CONTEXT.userSrc); | ||
await fs.remove(CONTEXT.userCfgRootPath); | ||
await fs.remove(CONTEXT.publicDir); | ||
await fs.remove(CONTEXT.scratchDir); | ||
}); | ||
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
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. can we set the indentation to 2 here? 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. yea I don't know why my editor defaults to 4, have to get around to changing that. |
||
"source": "src3", | ||
"publicPath": "/", | ||
"devServer": { | ||
"port": 1984, | ||
"host": "localhost" | ||
}, | ||
"meta": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,14 @@ module.exports = class Setup { | |
init() { | ||
return new Promise(async(resolve, reject) => { | ||
try { | ||
const ctx = await initContext(); | ||
const ctx = await initContext({ config: {} }); | ||
const context = { | ||
...ctx, | ||
userSrc: path.join(__dirname, '..', 'src'), // static src | ||
userTemplates: path.join(__dirname, '..', 'src', 'templates'), // static src/templates for testing empty templates dir, redundant in #38 | ||
testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src') | ||
testApp: path.join(__dirname, 'fixtures', 'mock-app', 'src'), | ||
userCfgPath: path.join(__dirname, 'fixtures', 'greenwood.config.json'), | ||
userCfgRootPath: path.join(__dirname, '..', 'greenwood.config.json') | ||
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. how come we are using this second config file? The names are really close and hard to distinguish. Maybe we just need more fixtures? My comment here about creating "cases" of tests might be an appropriate justification. 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. second config is just the hardcoded path to where the config will be copied. Hence: 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. ah, so basically for mocking the user's project root, where their own greenwood.config.json would be located? |
||
}; | ||
|
||
resolve(context); | ||
|
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.
probably not for this issue, but maybe we should create an issue to change init.js to context.js?
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.
Yes I agree with that.
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.
done
#57