Skip to content
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

Open
wants to merge 65 commits into
base: master
Choose a base branch
from

Conversation

lmgyuan
Copy link
Collaborator

@lmgyuan lmgyuan commented Apr 16, 2024

Description

  • add noOverwriteGlobs parameters to methods saveContentToFile and saveRenderedReactContent
  • pass this.noOverwriteGlobs to saveContentToFile
  • check the name of file to be written against the noOverwriteGlobs to determine whether it should be actually written to the target directory
  • add additional comments to relevant codes in the codebase so they are easier for future maintainers and contributors to understand.

Related issue(s)

Fixes #1128

@lmgyuan lmgyuan changed the title fix: Fix no overwrite globs #1128 fix: fix no overwrite globs #1128 Apr 16, 2024
- give noOverwriteGlobs a default value of [] in saveContentToFile and saveRenderedReactContent
@derberg derberg changed the title fix: fix no overwrite globs #1128 feat: support noOverwriteGlobs for templates based on react Apr 17, 2024
Copy link
Member

@derberg derberg left a 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?

@lmgyuan
Copy link
Collaborator Author

lmgyuan commented Apr 17, 2024

@derberg Thanks for your comments and guidance on adding a test to the test/integration.test.js! I agree that we could add a test so we can check whether noOverwriteGlobs is working when we use React Engine. However, I am a bit confused about how to use the template-for-generator-templates. You seem to suggest that I could use https://github.com/asyncapi/template-for-generator-templates react-based template as a base for test, so do you mean something like the following:

const generator = new Generator('https://github.com/asyncapi/template-for-generator-templates.git#328', outputDir, {
      forceWrite: true,
      noOverwriteGlobs: ['**/schema.js', '**/anotherFile.js'] // Specify files to ignore
    });

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

<File name={`noOverwrite_test.html`}>
        <SchemaFile schemaName={schemaName} schema={schema} />
      </File>

so after I use the generator template to generate files, I can check that it does not overwrite noOverwrite_test.html (with --noOverwriteGlobs nOverwrite_test as a command line argument). But how do I use the template-for-generator-templates to generate the output template with instructions on file name in the first place?

Currently, instead of using template-for-generator-template, I am thinking maybe the following codes could work as a test for noOverwriteGlobs feature as well:

it('should ignore specified files with noOverwriteGlobs', async () => {
    const outputDir = generateFolderName();
    // Manually create a file to test if it's not overwritten
    const testFilePath = path.join(outputDir, 'index.html');
    // eslint-disable-next-line sonarjs/no-duplicate-string
    writeFileSync(testFilePath, '<script>const initialContent = "This should not change";</script>');

    // based on the html-template documentation, the default output is index.html
    const generator = new Generator('@asyncapi/html-template@0.28.0', outputDir, {
      forceWrite: true,
      outFilename: 'index.html',
      noOverwriteGlobs: ['**/index.html']
    });

    generator.generateFromFile(dummySpecPath);
    
    // Read the file to confirm it was not overwritten
    const fileContent = readFileSync(testFilePath, 'utf8');
    expect(fileContent).toBe('<script>const initialContent = "This should not change";</script>');
  });

Let me know what you think! : )

@derberg
Copy link
Member

derberg commented Apr 18, 2024

You seem to suggest that I could use https://github.com/asyncapi/template-for-generator-templates react-based template as a base for test, so do you mean something like the following:

yup exactly, just use last commit from that repo, like this

https://github.com/asyncapi/template-for-generator-templates.git#f8353637ca6bc2b4eaa5514ceb3bd06fe145e9c6

but @asyncapi/html-template@0.28.0 is good too, especially that we use it in other test cases. I like your test suggestion 💯 especially that even though 1 file is ignored, with '**/index.html' globs are still tested

go ahead with implementation please

@lmgyuan
Copy link
Collaborator Author

lmgyuan commented Apr 18, 2024

@derberg just update the implementation!

lib/renderer/react.js Outdated Show resolved Hide resolved
@derberg
Copy link
Member

derberg commented Apr 29, 2024

@lmgyuan you'll need to solve conflicts first before you amend your integration tests

lmgyuan added 20 commits May 20, 2024 13:04
- revert changes to log messages
- add a new log test to see if the shouldOverwrite() is called
- generator.js: add a log to test if the conditional is ever true
- generator.js: add a variable to a log.debug() to check if the file path is correct
- integration test: add a new expect log test
- delete the log debug in shouldOverwriteFile
- in react, add a log debug to see if the problem is with variable
- mock imported log instead of console.log
- generator.js: setLogLevel in the construction of the generator
- integration.test: use log.debug = jest.fn() and expect(log.debug)
- don't reuse methods from generator for now
- use spyOn instead of jest.fn() in integration.test.js
- use regex (?!\\/loglevel\\.js$) in package.json
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'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this?

Copy link
Collaborator Author

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(() => {});
Copy link
Member

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

Copy link
Collaborator Author

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.

Comment on lines 98 to 101
// 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));
Copy link
Member

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


const outputDir = generateFolderName();
// Manually create a file to test if it's not overwritten
if (!await exists(outputDir)) {
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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'

Copy link

sonarcloud bot commented Jun 11, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

noOverwriteGlobs will not work in majority of cases in templates using react
2 participants