-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
feat: support noOverwriteGlobs
for templates based on react
#1191
Conversation
merge updates in asyncapi/generator master branch
…n/generator into fix-noOverwriteGlobs-1128
- give noOverwriteGlobs a default value of [] in saveContentToFile and saveRenderedReactContent
noOverwriteGlobs
for templates based on react
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.
@lmgyuan it's looking very good, now we just need a proper test to make sure it works long term.
Maybe we can add a test to test/integration.test.js
🤔
You could use https://github.com/asyncapi/template-for-generator-templates react-based template as a base for test. Generator template installation is npm based, so all features are included, so installing from GitHub link, pointing to specific commit will work (yes, we should not point to master to have stable tests).
This template generates a number of files -> https://github.com/asyncapi/template-for-generator-templates/blob/master/template/schemas/schema.js#L13-L20
So your test could generate output with noOverwriteGlobs
ignoring some of them, and test evaluation would be as simple reading output dir and making sure that some filenames are not there. That should be reliable enough
Thoughts?
@derberg Thanks for your comments and guidance on adding a test to the
There is not a specific reason for picking "#328" as the commit that this test is based on. I just picked one out of all the commits there are. I can see from the codes that the template generates a template that will generate some files, but may I ask how I could give it parameters so its output contains instructions about specific files that I want it to generate? For example, if I want the output generator template to have something like
so after I use the generator template to generate files, I can check that it does not overwrite Currently, instead of using
Let me know what you think! : ) |
yup exactly, just use last commit from that repo, like this
but go ahead with implementation please |
@derberg just update the implementation! |
@lmgyuan you'll need to solve conflicts first before you amend your integration tests |
test/integration.test.js
Outdated
return path.resolve(mainTestResultPath, crypto.randomBytes(4).toString('hex')); | ||
}; | ||
|
||
jest.setTimeout(60000); | ||
const testOutputFile = 'test-file.md'; | ||
|
||
beforeAll(() => { | ||
if (!existsSync(path.join(reactTemplate, 'package.json'))) { |
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.
why we need this?
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.
Deleted. I used it during development because I could not find the package.json sometimes.
@@ -55,4 +68,39 @@ describe('Integration testing generateFromFile() to make sure the result of the | |||
const file = await readFile(path.join(outputDir, testOutputFile), 'utf8'); | |||
expect(file).toMatchSnapshot(); | |||
}); | |||
|
|||
it('should ignore specified files with noOverwriteGlobs', async () => { | |||
const logSpyDebug = jest.spyOn(log, 'debug').mockImplementation(() => {}); |
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.
why not log.debug = jest.fn();
and then expect(log.debug)
, then you do not need to do mockRestore()
imho
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.
I tried that. jest.fn() does not work but spyOn() does. The former does not actually track the log.debug() calls in other files, but the latter does by creating a layer of observation around the existing methods.
test/integration.test.js
Outdated
// Check if the files have been overwritten | ||
await expect(fileContent).toBe(testContent); | ||
// Check if the log debug message was printed | ||
await expect(logSpyDebug).toHaveBeenCalledWith(logMessage.skipOverwrite(testFilePath)); |
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.
expect is not async call, so no need for await
test/integration.test.js
Outdated
|
||
const outputDir = generateFolderName(); | ||
// Manually create a file to test if it's not overwritten | ||
if (!await exists(outputDir)) { |
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.
actually I don't get why we need to validate if it exists, if a line earlier it is created
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.
const generateFolderName = () => {
// you always want to generate to new directory to make sure test runs in clear environment
// normalize the path for cross-platform compatibility
// return path.normalize(path.resolve(mainTestResultPath, crypto.randomBytes(4).toString('hex')));
return path.resolve(mainTestResultPath, crypto.randomBytes(4).toString('hex'));
};
If I understand it correctly, generateFolderName()
does not actually generate the directory, but just creates and returns a path string. Thus I manually checks and creates it.
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.
I see that in other tests you have written, you did not check and create the directory. Did you not have directory or path not found issue? If I comment out line 78-80, I have
ENOENT: no such file or directory, open '/Users/yuanyuan/workspace/generator/test/temp/integrationTestResult/6224bfdd/test-file.md'
Add console.log for all the spied method calls
delete unused existsSync
delete console.log() and use readFileSync
Quality Gate passedIssues Measures |
Quality Gate passedIssues Measures |
continued and merged in #1234 |
Description
noOverwriteGlobs
parameters to methodssaveContentToFile
andsaveRenderedReactContent
this.noOverwriteGlobs
tosaveContentToFile
noOverwriteGlobs
to determine whether it should be actually written to the target directoryRelated issue(s)
Fixes #1128