test: add template verification tests #205
Conversation
test/ci/verify-templates.test.js
Outdated
| const fs = require('fs'); | ||
| const { parser } = require('configure-env'); | ||
|
|
||
| const excludedPaths = ['node_modules', 'test', 'coverage', 'docs']; |
There was a problem hiding this comment.
should this also include blank, and possibly some of the other apps that are not quick deployable yet?
There was a problem hiding this comment.
I like including blank in this list, since it's intentionally non-QD. For the rest, I want to define a skipList or similar array that will contain the names of templates that aren't QD yet, but will be down the line. Eventually skipList will be empty, and we'll remove it from the test suite.
test/ci/verify-templates.test.js
Outdated
| }); | ||
|
|
||
| it('should be parseable', async () => { | ||
| const result = parser.parseFile(envFile); |
There was a problem hiding this comment.
should this be const result = await parser.parseFile(envFile); and then remove the await before the expect
There was a problem hiding this comment.
They're synonymous, but I changed it to your suggestion for consistency with the other .env tests.
test/ci/verify-templates.test.js
Outdated
| const result = await parser.parseFile(envFile); | ||
|
|
||
| result.variables.forEach((v) => { | ||
| if(!v.description) { |
There was a problem hiding this comment.
any reason not to use the expect framework here? something like
result.variables.forEach((v) => expect(v.description).toBeTruthy())
There was a problem hiding this comment.
I did it this way to get Jest to print a custom error message that calls out the specific variable name that doesn't have a description, since the default assert error is a bit confusing without it. Otherwise, there's unfortunately no way to get expect to report a custom error message out of the box. I can pull in one of the Jest plugins that adds this capability if you'd prefer a more Jest-like solution; I didn't know if this was a pattern we wanted to use elsewhere, so I hesitated initially to pull in a devDependency for this.
test/ci/verify-templates.test.js
Outdated
| const result = await parser.parseFile(envFile); | ||
| const varRegex = /^TWILIO_.*_WEBHOOK_URL$/; | ||
|
|
||
| result.variables |
There was a problem hiding this comment.
would it make more sense to filter for values matching the regex pattern then check for those configurable values to be falsy? Something like
result.variables
.filter((v) => varRegex.test(v.key))
.forEach((v) => expect(v.configurable).toBeFalsy())
There was a problem hiding this comment.
You're right, this is cleaner and probably faster.
test/ci/verify-templates.test.js
Outdated
| // These tests run in a CI context and verify that some important guarantees | ||
| // hold for all the functions in the repository. | ||
|
|
||
| if(process.env.CI) { |
There was a problem hiding this comment.
What was your thinking around not making these not as part of the standard npm test suite?
I feel like in some ways this might be more annoying to a developer to think they've passed all their tests locally, only to fail in the pr due to some mysterious suite of tests
There was a problem hiding this comment.
My initial understanding of this work was that these would run just for PR validation, probably to avoid slowing down the main test suite too much. That said, on my local system these execute in less than a second; I can definitely get behind adding these to the main test suite (possibly replacing all-templates.test.js with this file) if there's no objections to having them there.
also makes verifications run on every `npm test`, not just if CI=1
| (file) => | ||
| file.isDirectory() && | ||
| !file.name.startsWith('.') && | ||
| !file.name.startsWith('_') && | ||
| !excludedPaths.includes(file.name) |
There was a problem hiding this comment.
Seems like the indentation in this file is 4 and I think the rest of the project should be two if I'm not mistaken.
| fs.access(envFile, fs.constants.F_OK, (err) => { | ||
| expect(err).toBeFalsy(); | ||
| done(); | ||
| describe('CI template verification', () => { |
There was a problem hiding this comment.
nit: remove CI from the description here since its now part of the general test suite
jmulcahey-twilio
changed the title
dkundel
changed the title
Description
Defines a set of CI-only template verifications that run for all of the apps in the repository. The following properties are checked:
functionssubdirectory exists and contains at least one file.assets/index.htmlexists.testssubdirectory exists and contains at least one file..envfile exists, is parseable, and all of its variables have adescriptionfield, and any variables namedTWILIO_*_WEBHOOK_URLare not configurable.package.jsonexists in the root of the app, and its contents are parseable as a Javascript object literal of some form.assets/index.html, an attempt is made to detect new-styleindex.htmlfiles (by searching them force-paste-theme.css) and ensuring they contain anAPP_INFO_V2comment.I decided to implement these verifications on top of Jest, with the tests enabled unless the environment variable
CI=1. Jest had some advantages that made it attractive for this use case:To prevent these verifications from running on every invocation of
npm testorjest, I've gated them behind an environment variable, which seems to be one of the only mechanisms Jest supports for enabling and disabling test suites dynamically. The current choice ofCI=1ensures the CI's currentnpm teststep will run these additional tests, but if we want more granular control over when they run, we can change the environment variable to something else (IE,TWILIO_CI_VERIFY=1) and use thenpm run ci:verify-templatesscript I've defined to run these verification tasks.Checklist
npm testlocally and it passed without errors.Related issues